On Tue, 10 Sep 2024 18:35:52 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
> Please review this PR which speeds up `ZipFile.getZipEntry` by removing
> slash-checking logic which is already taking place during lookup in
> `ZipFile.Source.getEntryPos`.
>
> `ZipFile.Source.getEntryPos` includes logic to match a lookup for "name"
> against a directory entry "name/" (with a trailing slash). However, only the
> CEN position is currently returned, so `ZipFile.getZipEntry` needs to re-read
> the name from the CEN and determine if a trailing slash needs to be appended
> to the name of the returned `ZipEntry`.
>
> By letting `ZipFile.Source.getEntryPos` return the resolved name along with
> the CEN position (in a new record `EntryPos`), `ZipFile.getZipEntry` can now
> instead use the already resolved name.
>
> This results in a nice ~18% speedup in the `ZipFileGetEntry.getEntryHit`
> micro:
>
> Baseline:
>
>
> Benchmark (size) Mode Cnt Score Error Units
> ZipFileGetEntry.getEntryHit 512 avgt 15 63.713 ? 2.645 ns/op
> ZipFileGetEntry.getEntryHit 1024 avgt 15 67.405 ? 1.474 ns/op
>
>
> PR:
>
>
> Benchmark (size) Mode Cnt Score Error Units
> ZipFileGetEntry.getEntryHit 512 avgt 15 52.027 ? 2.669 ns/op
> ZipFileGetEntry.getEntryHit 1024 avgt 15 55.211 ? 1.169 ns/op
>
>
> This purely a cleanup and optimization PR, no functional tests are changed or
> added.
Some initial thoughts but overall it seems to be a beneficial change.
src/java.base/share/classes/java/util/zip/ZipCoder.java line 161:
> 159: }
> 160:
> 161: protected boolean hasTrailingSlash(byte[] a, int end) {
Can you explain the need for this
src/java.base/share/classes/java/util/zip/ZipFile.java line 700:
> 698: }
> 699: if (clen != 0) {
> 700: int nlen = CENNAM(cen, pos);
If this does not make a huge performance difference, I would keep this together
with the init of elen and clen on line 692.
While there are not many entry comments encountered, I do expect extra fields
to somewhat common
-------------
PR Review: https://git.openjdk.org/jdk/pull/20939#pullrequestreview-2293604516
PR Review Comment: https://git.openjdk.org/jdk/pull/20939#discussion_r1752629986
PR Review Comment: https://git.openjdk.org/jdk/pull/20939#discussion_r1752620013
