8341594: Use Unsafe to coalesce reads in java.util.zip.ZipUtils #21377

cl4es · 2024-10-06T14:16:44Z

#14632 showed that coalescing loads in the ZipUtils utility methods could improve performance in zip-related microbenchmarks, but the suggested PR would increase startup overheads by early use of ByteArrayLittleEndian which depends on VarHandles. Progress was stalled as we backed out some related early use of ByteArray(LittleEndian) and started exploring merge store optimizations in C2.

In this PR I instead suggest using Unsafe directly to coalesce short, int, and long reads from zip data. Even with explicit bounds checking to ensure these utilities are always safe there are significant improvements both to lookup speed and speed of opening zip files (most if not all bounds checks are optimized away):

make test TEST=micro:java.util.zip.ZipFile

Name                          (size) Cnt       Base      Error        Test      Error  Unit  Change
GetEntry.getEntryHit             512  15     37.999 ±    0.841      34.641 ±    0.389 ns/op   1.10x (p = 0.000*)
GetEntry.getEntryHit            1024  15     39.557 ±    0.523      36.959 ±    1.488 ns/op   1.07x (p = 0.000*)
GetEntry.getEntryHitUncached     512  15     69.250 ±    0.931      64.851 ±    0.987 ns/op   1.07x (p = 0.000*)
GetEntry.getEntryHitUncached    1024  15     71.628 ±    0.307      67.927 ±    0.714 ns/op   1.05x (p = 0.000*)
GetEntry.getEntryMiss            512  15     22.961 ±    0.336      22.825 ±    0.188 ns/op   1.01x (p = 0.158 )
GetEntry.getEntryMiss           1024  15     22.940 ±    0.115      23.502 ±    0.273 ns/op   0.98x (p = 0.000*)
GetEntry.getEntryMissUncached    512  15     35.886 ±    0.429      35.598 ±    1.296 ns/op   1.01x (p = 0.395 )
GetEntry.getEntryMissUncached   1024  15     38.168 ±    0.911      36.141 ±    0.356 ns/op   1.06x (p = 0.000*)
Open.openCloseZipFile            512  15  62425.563 ±  997.455   56263.401 ±  896.892 ns/op   1.11x (p = 0.000*)
Open.openCloseZipFile           1024  15 117491.250 ±  962.928  108055.491 ± 1595.577 ns/op   1.09x (p = 0.000*)
Open.openCloseZipFilex2          512  15  62974.575 ±  911.095   57996.388 ±  910.929 ns/op   1.09x (p = 0.000*)
Open.openCloseZipFilex2         1024  15 119164.769 ± 1756.065  108803.468 ±  929.483 ns/op   1.10x (p = 0.000*)
  * = significant

This PR also address some code duplication in ZipUtils.

An appealing alternative would be to implement a merge load analogue to the merge store optimizations in C2. Such optimizations would be very welcome since it would improve similar code outside of java.base (jdk.zipfs has some duplicate code that is left untouched) and reduce the need for Unsafe trickery. This enhancement and the microbenchmarks could then be used as verification and the Unsafe code backed out.

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-8341594: Use Unsafe to coalesce reads in java.util.zip.ZipUtils (Enhancement - P4)

Reviewers

Lance Andersen (@LanceAndersen - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21377

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-06T14:17:16Z

👋 Welcome back redestad! 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-10-06T14:17:47Z

@cl4es 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:

8341594: Use Unsafe to coalesce reads in java.util.zip.ZipUtils

Reviewed-by: lancea

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 103 new commits pushed to the master branch:

fc7244d: 8340713: Open source DnD tests - Set5
f7bb647: 8341595: Clean up iteration of CEN headers in ZipFile.Source.initCEN
d0c5e4b: 8341373: Open source closed frame tests # 4
3359518: 8341593: Problemlist java/foreign/TestUpcallStress.java in Xcomp mode
a2372c6: 8341238: G1: Refactor G1Policy to move collection set selection methods into G1CollectionSet
4ba170c: 8341235: Improve default instruction frame title in PassFailJFrame
520060f: 8340799: Add border inside instruction frame in PassFailJFrame
2897797: 8340880: RISC-V: add t3-t6 alias into assemler_riscv.hpp
747a3fa: 8341562: RISC-V: Generate comments in -XX:+PrintInterpreter to link to source code
81ebbb2: 8341525: G1: use bit clearing to remove tightly-coupled initialization store pre-barriers
... and 93 more: https://git.openjdk.org/jdk/compare/ad5ffccffa89359dac6ad44b9e43242e5bf3e398...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-10-06T14:19:10Z

@cl4es 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-10-06T14:23:12Z

Webrevs

liach · 2024-10-06T14:40:37Z

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

+        Preconditions.checkIndex(off, b.length, Preconditions.AIOOBE_FORMATTER);
+        Preconditions.checkIndex(off + 1, b.length, Preconditions.AIOOBE_FORMATTER);


Please use Preconditions.checkFromIndexSize, which should be less overhead:

Suggested change

Preconditions.checkIndex(off, b.length, Preconditions.AIOOBE_FORMATTER);

Preconditions.checkIndex(off + 1, b.length, Preconditions.AIOOBE_FORMATTER);

Preconditions.checkFromIndexSize(off, 2, b.length, Preconditions.AIOOBE_FORMATTER);

Similarly for other methods.

It's actually not less overhead in my tests, since checkIndex is intrinsic and mostly disappears, while with checkFromIndexSize performance gets significantly worse (on par with baseline). It's on my todo to investigate this in-depth but I think checkFromIndexSize needs to be given similar intrinsification treatment as checkIndex.

Actually if we trust the input index to be nonnegative, we can just check our end index for out of bounds too.

Sure, I think the JIT is pretty good at eliminating the (intrinsic) checkIndex calls when they are redundant though. Performance with and without these checkIndexes are the same in my testing, so we can eat and have the cake on this one.

FWIW I wouldn't mind giving similar treatment to ByteArray(-LittleEndian) and avoid the VarHandles dependency in those utility classes, but I have no urge to get into the sort of discussions that were spawned in #19616

Actually if we trust the input index to be nonnegative, we can just check our end index for out of bounds too.

I would not trust that. Perhaps for well-formed ZIP files, but trust me, not all ZIPs are well-formed ;-)

Yep, that requires you to pre-validate the input argument as a fixed position after already-read content, which is not always the case.

Like @eirbjo suggests we'd have to put a lot of validation in other places if we went down this route. Regardless this is an academic discussion since the PR suggests the safe route and we don't pay much of a cost for that in microbenchmarks.

test/micro/org/openjdk/bench/java/util/zip/ZipFileOpen.java

eirbjo · 2024-10-06T14:56:27Z

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

-                        buf[i+1] == (byte)'K'    &&
-                        buf[i+2] == (byte)'\005' &&
-                        buf[i+3] == (byte)'\006') {
+                    if (get32(buf, i) == ENDSIG) {


Maybe a matter of personal preference, but I think GETSIG(buf, i) == ENDSIG reads better than get32(buf, i) == ENDSIG.

The fact that it's 32 bits is kind of a detail and it doesn't reveal as well that we intend to read a signature.

So could we keep GETSIG, but add an index? There are places in ZipInputStream as well which could make use of that for signature checking. (But maybe not for this PR)

Alternatively, ENDSIG(buf, i) == ENDSIG would be consistent with CENSIG(buf, i) uses.

Same applies to the other GETSIG replacements in this file.

I think all the GETSIG(byte[]) methods are quite nasty, and it's all used very inconsistently. I wouldn't mind going the other way and removing all the CENSIG, LOCNAM etc methods and just call get16/32/32S/64(buf, ZipConstants.FOO) as appropriate. Add a comment at each ZipConstants entry about exactly how many bytes are in the field, if it's unsigned or signed. Then at least there's a reference to something that looks like a specification, not a mix of constants and single-purpose offset getters scattered around (which doesn't even reference the constants in ZipConstants).

eirbjo · 2024-10-06T15:27:07Z

test/micro/org/openjdk/bench/java/util/zip/ZipFileOpen.java

        zf.close();
        zf2.close();
    }
+


A short comment stating the purpose of the main method would not hurt.

#19477 (comment)

This applies to many benchmarks, so I wonder where is the best place for such a note.

I think it's fair to add a descriptive comment individually.

eirbjo · 2024-10-06T15:29:13Z

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

-    static final long CENOFF(byte[] b, int pos) { return LG(b, pos + 42);}
+    static final long CENSIG(byte[] b, int pos) { return get32(b, pos + 0); }
+    static final int  CENVEM(byte[] b, int pos) { return get16(b, pos + 4); }
+    static final int  CENVEM_FA(byte[] b, int pos) { return Byte.toUnsignedInt(b[pos + 5]); } // file attribute compatibility


Did you consider introducing get8 for consistency here? As it stands, this looks like the odd one out.

I considered it, but since get8 would basically just delegate to or do exactly what Byte.toUnsignedInt does I opted to cut out the middle man.

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

eirbjo · 2024-10-06T15:53:55Z

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

     * Fetches signed 64-bit value from byte array at specified offset.
     * The bytes are assumed to be in Intel (little-endian) byte order.
     */
    public static final long get64(byte[] b, int off) {


This method returns a signed 64 bit value, which I think is not what some of its call sites expect. It should in any case be renamed to get64S to align with get32S. A new method get64 should be introduced and any call site expecting unsigned numbers (most?) should use that instead.

If you don't want to deal with this in this PR, I could file an issue and suggest a PR for this. Let me know.

As it's a pre-existing issue I'd prefer to keep this one focused on the switch-over. How would you model unsigned long values here, though? Sure we could read into a BigInteger or accept negative values, but to really support such overflows we might have to rework a lot of things.

FWIW we already cap some values even lower in practice:

end.centot = (int)centot64; // assume total < 2g

How would you model unsigned long values here, though?

I don't think we should. 9223372036854775807 should be enough for everyone :-)

It may be worth renaming the method to get64S and add a get64 variant which either clamps at LONG.MAX_VALUE or throws IllegalArgumentException for larger values. Call sites doing custom validation (like checkZip64ExtraFieldValues) could then call get64S and check for a negative long.

But that's food for another PR.

Renaming to get64S is reasonable to be internally consistent. Updated. Improving validation of data in such 64-bit fields I'll leave for the future. I think a reasonable stance is to throw in the check methods if any such field is negative, at least for some of these fields.

FWIW we already cap some values even lower in practice:

end.centot = (int)centot64; // assume total < 2g

I submitted #21384 which adds validation of end.centot and also eliminates this narrowing conversion.

LanceAndersen

Hi Claes,

Looks reasonable to me.

thank you for your efforts here

wenshao · 2024-10-08T01:05:14Z

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

     * Fetches unsigned 16-bit value from byte array at specified offset.
     * The bytes are assumed to be in Intel (little-endian) byte order.
     */
    public static final int get16(byte[] b, int off) {


Can JIT automatically perform MergeStore here? If JIT can automatically perform MergeStore without using Unsafe, many scenarios will benefit.

Unfortunately we only have merge store for writing to arrays; no merge load when we are reading from arrays.

Yes. As I wrote in the PR description: "An appealing alternative would be to implement a merge load analogue to the merge store optimizations in C2." So it's the opposite of a merge store. I recall @eme64 mentioning somewhere that such an optimization would be possible, if we could show it to be profitable. I think it's reasonable to do this enhancement now, then revert the Unsafe stuff if/when there's a merge load optimization that beats it.

cl4es · 2024-10-08T08:14:02Z

/integrate

openjdk · 2024-10-08T08:14:43Z

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

57c859e: 8339836: Open source several AWT Mouse tests - Batch 1
b6a4047: 8339982: Open source several AWT Mouse tests - Batch 2
45a6359: 8341668: Shenandoah: assert(tail_bits < (idx_t)BitsPerWord) failed: precondition
d996ca8: 8341581: Optimize BytecodeHelpers validate slot
4d50cbb: 8341278: Open source few TrayIcon tests - Set7
23f3ca2: 8330206: Bump minimum boot jdk to JDK 23
fc7244d: 8340713: Open source DnD tests - Set5
f7bb647: 8341595: Clean up iteration of CEN headers in ZipFile.Source.initCEN
d0c5e4b: 8341373: Open source closed frame tests # 4
3359518: 8341593: Problemlist java/foreign/TestUpcallStress.java in Xcomp mode
... and 99 more: https://git.openjdk.org/jdk/compare/ad5ffccffa89359dac6ad44b9e43242e5bf3e398...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-08T08:14:48Z

@cl4es Pushed as commit ffb60e5.

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

cl4es added 3 commits October 6, 2024 13:40

Use Unsafe to coalesce reads in java.util.zip.ZipUtils

c4b327a

Remove ForceInline after verification it's not needed

9ffaff1

Add main method to ZipFileOpen, enabling use as a startup benchmark

32010f6

openjdk bot added the rfr Pull request is ready for review label Oct 6, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Oct 6, 2024

liach suggested changes Oct 6, 2024

View reviewed changes

eirbjo reviewed Oct 6, 2024

View reviewed changes

test/micro/org/openjdk/bench/java/util/zip/ZipFileOpen.java Show resolved Hide resolved

copyright

d904ce6

eirbjo suggested changes Oct 6, 2024

View reviewed changes

eirbjo reviewed Oct 6, 2024

View reviewed changes

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

eirbjo reviewed Oct 6, 2024

View reviewed changes

cl4es added 2 commits October 6, 2024 21:20

Address review comments

23f0c8e

Rename get64 -> get64S

360afea

LanceAndersen approved these changes Oct 7, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 7, 2024

wenshao reviewed Oct 8, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 8, 2024

openjdk bot closed this Oct 8, 2024

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

		Preconditions.checkIndex(off, b.length, Preconditions.AIOOBE_FORMATTER);
		Preconditions.checkIndex(off + 1, b.length, Preconditions.AIOOBE_FORMATTER);

	Preconditions.checkIndex(off, b.length, Preconditions.AIOOBE_FORMATTER);
	Preconditions.checkIndex(off + 1, b.length, Preconditions.AIOOBE_FORMATTER);
	Preconditions.checkFromIndexSize(off, 2, b.length, Preconditions.AIOOBE_FORMATTER);

8341594: Use Unsafe to coalesce reads in java.util.zip.ZipUtils #21377

8341594: Use Unsafe to coalesce reads in java.util.zip.ZipUtils #21377

Uh oh!

Conversation

cl4es commented Oct 6, 2024 • edited 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 Oct 6, 2024

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Oct 6, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cl4es Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

cl4es Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cl4es Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cl4es Oct 6, 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!

eirbjo Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cl4es commented Oct 6, 2024 •

edited

Loading

openjdk bot commented Oct 6, 2024 •

edited

Loading

mlbridge bot commented Oct 6, 2024 •

edited

Loading

cl4es Oct 6, 2024 •

edited

Loading

cl4es Oct 6, 2024 •

edited

Loading

cl4es Oct 6, 2024 •

edited

Loading

cl4es Oct 6, 2024 •

edited

Loading

eirbjo Oct 6, 2024 •

edited

Loading