RFR: 8339874: Avoid duplicate checking of trailing slash in ZipFile.getZipEntry

Claes Redestad redestad at openjdk.org
Tue Sep 10 19:42:04 UTC 2024


On Tue, 10 Sep 2024 18:35:52 GMT, Eirik Bjørsnøs <eirbjo at 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.

src/java.base/share/classes/java/util/zip/ZipFile.java line 681:

> 679: 
> 680:         e.flag = CENFLG(cen, pos);
> 681:         e.method = CENHOW(cen, pos);

Not reading `nlen` when it's not needed is a good change, and moving `clen` and `elen` down to be grouped with the others is fine, but some of the other shuffling around here doesn't seem motivated?

src/java.base/share/classes/java/util/zip/ZipFile.java line 1891:

> 1889:             // Return the position of "name/" if we found it
> 1890:             if (dirPos != -1) {
> 1891:                 return new EntryPos(name +"/", dirPos);

Suggestion:

                return new EntryPos(name + "/", dirPos);

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20939#discussion_r1752630171
PR Review Comment: https://git.openjdk.org/jdk/pull/20939#discussion_r1752628607


More information about the core-libs-dev mailing list