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

eirbjo · 2024-09-10T18:35:52Z

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.

Since ZipFile.getZipEntry now has the name, CEN header fields can now be read in bulk, separate from the allocation of the ZipEntry. This reordering is unlocked by the other changes in this PR and can alone explain a lot of the performance gains, probably because of better cache use.

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

The changes in this PR makes UTF8ZipCoder.compare the only caller of ZipCoder.hasTrailingSlash, so this method is made private and the implementation in the base class retired.

This purely a cleanup and optimization PR, no functional tests are changed or added.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8339874: Avoid duplicate checking of trailing slash in ZipFile.getZipEntry (Enhancement - P4)

Reviewers

Lance Andersen (@LanceAndersen - Reviewer)
Claes Redestad (@cl4es - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20939/head:pull/20939
$ git checkout pull/20939

Update a local copy of the PR:
$ git checkout pull/20939
$ git pull https://git.openjdk.org/jdk.git pull/20939/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20939

View PR using the GUI difftool:
$ git pr show -t 20939

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20939.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-10T18:36:48Z

👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-09-10T18:37:03Z

@eirbjo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: lancea, redestad

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 43 new commits pushed to the master branch:

1b17e0b: 8338747: hasIncubatorModules needs to be generated when module resolution required at startup
3c40afa: 8334165: Remove serialVersionUID compatibility logic from JMX
315abdf: 8339733: C2: some nodes can have incorrect control after do_range_check()
ac3f92b: 8339731: java.desktop/share/classes/javax/swing/text/html/default.css typo in margin settings
cfbf74f: 8339159: api/java_rmi/Naming/Rebind.html crashes with SEGV from UTF8::quoted_ascii_length call
6d4bd6c: 8339835: Replace usages of -mx and -ms in some client-libs tests
1d39249: 8339834: Replace usages of -mx and -ms in some tests
c3711dc: 8339678: Update runtime/condy tests to be executed with VM flags
b0cff6b: 8299419: Thread.sleep(millis) may throw OOME
591aa7c: 8335362: [Windows] Stack pointer increment in _cont_thaw stub can cause program to terminate with exit code 0xc0000005
... and 33 more: https://git.openjdk.org/jdk/compare/77468c284c068f921da543edd28333911e915b61...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-09-10T18:37:29Z

@eirbjo The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-09-10T18:58:41Z

Webrevs

cl4es · 2024-09-10T19:34:11Z

src/java.base/share/classes/java/util/zip/ZipCoder.java

    }

-    boolean hasTrailingSlash(byte[] a, int end) {
+    protected boolean hasTrailingSlash(byte[] a, int end) {


Why are you making these protected? ZipCoder is package-private so any inheritors must be in the same package, which means they already have access to package-private methods.

Since hasTrailingSlash is now only used from UTFCoder and subclasses, I thought we could stricten the access to protected. But as I learned, protected is still accessible from the same package.

On closer inspection though, hasTrailingSlash is now used only from UTF8ZipEncoder.compare. So we can actually make that implementation private and remove the now-unused implementation from the base class, along with the slashBytes logic. I have pushed these changes.

What do you think?

src/java.base/share/classes/java/util/zip/ZipFile.java

cl4es · 2024-09-10T19:39:34Z

src/java.base/share/classes/java/util/zip/ZipFile.java

-            e = new ZipEntry(name);
-        }
        e.flag = CENFLG(cen, pos);
+        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?

The reordering of field reads was motivated by some previous experiences where ordering the reads sequentially with their appearance in the CEN a had small, but positive performance gain.

After closer investigation, it turns out that the internal ordering of field reads only has small benefits, maybe in the noise. However, reading the length fields before the allocation of the ZipEntry has a significant negative impact. In fact, it seems to explain most of the performance gains in this PR.

It seems that having ZipEntry allocation interspersed within the CEN field reads incurs a significant cost. I can't explain why, but perhaps @cl4es can? (To reproduce, simply move the length reads to the beginning of the method)

I have kept the reordering of nlen, elen, clen reads, but reverted some other reorderings to make the PR cleaner.

This PR:

Benchmark (size) Mode Cnt Score Error Units ZipFileGetEntry.getEntryHit 512 avgt 15 52.057 ? 2.021 ns/op ZipFileGetEntry.getEntryHit 1024 avgt 15 54.753 ? 1.093 ns/op

Reads length fields before ZipEntry allocation:

Benchmark (size) Mode Cnt Score Error Units ZipFileGetEntry.getEntryHit 512 avgt 15 65.199 ? 0.823 ns/op ZipFileGetEntry.getEntryHit 1024 avgt 15 69.407 ? 0.807 ns/op

Just a guess but perhaps this is down to a cache effect where the ZipEntry allocation has a chance to evict the cen data array from some cache. Batching all the reads from CEN together could counter-act some such effects and better streamline memory accesses.

LanceAndersen

Some initial thoughts but overall it seems to be a beneficial change.

src/java.base/share/classes/java/util/zip/ZipFile.java

src/java.base/share/classes/java/util/zip/ZipCoder.java

cl4es · 2024-09-10T19:57:56Z

src/java.base/share/classes/java/util/zip/ZipFile.java

                // each "entry" has 3 ints in table entries
-                return (T)getZipEntry(null, res.zsrc.getEntryPos(i++ * 3));
+                int pos = res.zsrc.getEntryPos(i++ * 3);
+                return (T)getZipEntry(new EntryPos(getEntryName(pos), pos));


Do we have benchmarks covering the various types of iteration?

I realized it's better to keep the signature of getZipEntry so these iteration callers won't need to construct the EntryPos. I assume calling getEntryName should have similar performance characteristic to the existing code which calls getEntryPos with a null name, since we're just seem to be moving the String decoding from one method to another.

But no, I don't think we have benchmarks for ZipEntry enumeration.

…d to wrap arguments in as EntryPos instances

…oder.hasTrailingSlash with the associated slashBytes() method and slashBytes field

…ew. Move nlen read to the common path.

LanceAndersen

thank you for the clean up Eirik, I think the changes look good overall.

eirbjo · 2024-09-12T11:30:33Z

For the benefit of future maintainers, I added a code comment where ZipFile::getEntry calls Source::getEntryPos, pointing out that the resolved name from the CEN may differ with a trailing slash.

This reminds me that getEntryPos is maybe not a great name. Perhaps something like lookupEntry would be more honest about the non-trivial logic involved. But perhaps that is for another PR.

cl4es

LGTM

eirbjo · 2024-09-12T15:22:24Z

/integrate

openjdk · 2024-09-12T15:24:24Z

Going to push as commit 7f1dae1.
Since your change was applied there have been 45 commits pushed to the master branch:

4d65c3e: 8339876: Move constant symbol caches to Utf8EntryImpl
0765917: 8340011: Simplify jdk.internal.classfile.impl.EntryMap
1b17e0b: 8338747: hasIncubatorModules needs to be generated when module resolution required at startup
3c40afa: 8334165: Remove serialVersionUID compatibility logic from JMX
315abdf: 8339733: C2: some nodes can have incorrect control after do_range_check()
ac3f92b: 8339731: java.desktop/share/classes/javax/swing/text/html/default.css typo in margin settings
cfbf74f: 8339159: api/java_rmi/Naming/Rebind.html crashes with SEGV from UTF8::quoted_ascii_length call
6d4bd6c: 8339835: Replace usages of -mx and -ms in some client-libs tests
1d39249: 8339834: Replace usages of -mx and -ms in some tests
c3711dc: 8339678: Update runtime/condy tests to be executed with VM flags
... and 35 more: https://git.openjdk.org/jdk/compare/77468c284c068f921da543edd28333911e915b61...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-12T15:24:30Z

@eirbjo Pushed as commit 7f1dae1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Avoid repeated checking of trailing slash in ZipFile.getZipEntry

6c9f09b

openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 10, 2024

eirbjo marked this pull request as ready for review September 10, 2024 18:54

openjdk bot added the rfr Pull request is ready for review label Sep 10, 2024

cl4es reviewed Sep 10, 2024

View reviewed changes

src/java.base/share/classes/java/util/zip/ZipFile.java Outdated Show resolved Hide resolved

cl4es reviewed Sep 10, 2024

View reviewed changes

LanceAndersen reviewed Sep 10, 2024

View reviewed changes

src/java.base/share/classes/java/util/zip/ZipFile.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/util/zip/ZipCoder.java Outdated Show resolved Hide resolved

cl4es reviewed Sep 10, 2024

View reviewed changes

eirbjo added 6 commits September 11, 2024 06:30

Revert change in access modifier for ZipCoder.checkedHash

dd0ac5e

Revert signature change in ZipFile.getZipEntry so iterators won't nee…

974306d

…d to wrap arguments in as EntryPos instances

Make UTF8ZipCoder.hasTrailingSlash private and remove the unused ZipC…

679ee58

…oder.hasTrailingSlash with the associated slashBytes() method and slashBytes field

Revert some field read ordering changes to make the PR easier to revi…

b6de843

…ew. Move nlen read to the common path.

Add whitespace per review feedback

f24b833

Add comment where getEntry calls Source::getEntryPos

427c34c

LanceAndersen approved these changes Sep 12, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 12, 2024

cl4es approved these changes Sep 12, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 12, 2024

openjdk bot closed this Sep 12, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 12, 2024

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

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

Uh oh!

Conversation

eirbjo commented Sep 10, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Sep 10, 2024

Uh oh!

openjdk bot commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 10, 2024

Uh oh!

mlbridge bot commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

cl4es Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

eirbjo Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cl4es Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

eirbjo Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cl4es Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cl4es Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

eirbjo Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

eirbjo commented Sep 12, 2024

Uh oh!

cl4es left a comment

Choose a reason for hiding this comment

Uh oh!

eirbjo commented Sep 12, 2024

Uh oh!

openjdk bot commented Sep 12, 2024

Uh oh!

openjdk bot commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

eirbjo commented Sep 10, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 10, 2024 •

edited

Loading

mlbridge bot commented Sep 10, 2024 •

edited

Loading

eirbjo Sep 11, 2024 •

edited

Loading

eirbjo Sep 11, 2024 •

edited

Loading