8300493: Use ArraysSupport.vectorizedHashCode in j.u.zip.ZipCoder #12077

cl4es · 2023-01-18T16:53:04Z

ZipCoder::checkedHashCode emulates StringLatin1::hashCode but operates on a byte[] subrange. It can profitably use the recently introduced ArraysSupport::vectorizedHashCode method to see a speed-up, which translates to a small but significant speed-up on ZipFile creation.

Before:

Benchmark                     (size)  Mode  Cnt       Score      Error  Units
ZipFileOpen.openCloseZipFile     512  avgt   15   83007.325 ± 1446.716  ns/op
ZipFileOpen.openCloseZipFile    1024  avgt   15  154550.631 ± 2166.673  ns/op

After:

Benchmark                     (size)  Mode  Cnt       Score      Error  Units
ZipFileOpen.openCloseZipFile     512  avgt   15   79512.902 ±  814.449  ns/op
ZipFileOpen.openCloseZipFile    1024  avgt   15  147892.522 ± 2744.017  ns/op

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-8300493: Use ArraysSupport.vectorizedHashCode in j.u.zip.ZipCoder

Reviewers

Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 0c7f0f4f
Lance Andersen (@LanceAndersen - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12077

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

Using diff file

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

bridgekeeper · 2023-01-18T16:54:20Z

👋 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 · 2023-01-18T16:58:05Z

@cl4es The following labels will be automatically applied to this pull request:

core-libs
security

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

mlbridge · 2023-01-18T17:02:07Z

Webrevs

01: Full - Incremental (369537c5)
00: Full (0c7f0f4f)

AlanBateman · 2023-01-18T18:39:37Z

Using countPositives, and vectorizedHashCode(T_BOOLEAN) for unsigned bytes, make sense here. I don't have time to study the micro right now.

cl4es · 2023-01-19T09:46:12Z

FWIW the micro is derived from the sibling ZipFileGetEntry micro in the same directory. It's not exactly necessary for this use case to add such a benchmark, but I think there's value in verifying that optimizing checkedHash improves ZipFile setup and adding the micro might allow us to find further opportunities down the line.

AlanBateman · 2023-01-20T11:10:13Z

FWIW the micro is derived from the sibling ZipFileGetEntry micro in the same directory. It's not exactly necessary for this use case to add such a benchmark, but I think there's value in verifying that optimizing checkedHash improves ZipFile setup and adding the micro might allow us to find further opportunities down the line.

I've no doubt it improves checkedHash, it's just that open the zip file and reading in the CEN probably dominates when not in the file system cache.

openjdk · 2023-01-20T11:12:42Z

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

8300493: Use ArraysSupport.vectorizedHashCode in j.u.zip.ZipCoder

Reviewed-by: alanb, 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 80 new commits pushed to the master branch:

92d8326: 8299827: Add resolved IP address in connection exception for sockets
c6d5600: 8038146: Clarify Map.Entry's connection to the underlying map
b2d3622: 8299896: Reduce enum values of HtmlLinkInfo.Kind
623ba5b: 8300653: G1EvacInfo should use common naming scheme for collection set
97c611d: 8289748: C2 compiled code crashes with SIGFPE with -XX:+StressLCM and -XX:+StressGCM
4562b40: 8300682: InstanceKlassMiscStatus is a bad name
26410c1: 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
eca6479: 8300087: Replace NULL with nullptr in share/cds/
49d60fe: 8300172: java/net/httpclient/MappingResponseSubscriber.java failed with java.net.ConnectException
e189397: 8296403: [TESTBUG] IR test runner methods in TestLongRangeChecks.java invoke wrong test methods
... and 70 more: https://git.openjdk.org/jdk/compare/89a032dc057d04c996632ad317a0303cf3560852...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.

LanceAndersen · 2023-01-20T11:25:22Z

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

+                zos.putNextEntry(new ZipEntry(ename));
+
+                ename += "long-entry-name-"  + (random.nextInt(90000) + 10000)  + "-" + i;
+                zos.putNextEntry(new ZipEntry(ename));


Would it be worth to add some random sized data when the entries are created to allow for getting a bit more insight, or perhaps do that in a separate benchmark>

I was experimenting with varying the entry names length to see what - if any - impact it had and saw a small effect on the micro. It does make more sense to vary lengths now that very long names will take different paths in the vectorized intrinsic. I'll see what I can do without overengineering this.

cl4es · 2023-01-20T11:43:17Z

FWIW the micro is derived from the sibling ZipFileGetEntry micro in the same directory. It's not exactly necessary for this use case to add such a benchmark, but I think there's value in verifying that optimizing checkedHash improves ZipFile setup and adding the micro might allow us to find further opportunities down the line.

I've no doubt it improves checkedHash, it's just that open the zip file and reading in the CEN probably dominates when not in the file system cache.

Right, the micro is a poor proxy for real-world implications since time to open a zip file very much depends on the filesystem speed but this is sort of by design. We have separate startup tests that tries to emulate more "cold start" scenarios, which micros like this are complementary to and not a substitute for.

…ntries short but making the longest paths longer

cl4es · 2023-01-20T12:14:21Z

Updated micro to vary entry sizes more to ensure we exercise the different code paths through the hashCode intrinsic. The new setup generates both longer and shorter entries than before, weighting up the average length a bit by increasing the spread. The longer entries see a proportionately larger speed-up, as expected since they benefit from vectorization. Removed some pointless randomness.

Baseline:

Benchmark                     (size)  Mode  Cnt       Score      Error  Units
ZipFileOpen.openCloseZipFile     512  avgt   15   98832.801 ± 2155.928  ns/op
ZipFileOpen.openCloseZipFile    1024  avgt   15  187373.545 ± 2296.779  ns/op

Patched:

Benchmark                     (size)  Mode  Cnt       Score      Error  Units
ZipFileOpen.openCloseZipFile     512  avgt   15   85574.648 ±  920.477  ns/op
ZipFileOpen.openCloseZipFile    1024  avgt   15  160493.277 ± 3450.928  ns/op

LanceAndersen

Thank you Claes

cl4es · 2023-01-21T11:50:24Z

Thanks @LanceAndersen and @AlanBateman for reviewing!

/integrate

openjdk · 2023-01-21T11:56:24Z

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

06394ee: 8300590: [JVMCI] BytecodeFrame.equals is broken
5331a3e: 8298908: Instrument Metaspace for ASan
e1ee672: 8300725: Improve performance of ColorConvertOp for default destinations with alpha
7c2f77a: 8300584: Accelerate AVX-512 CRC32C for small buffers
5784eb7: 8300721: Cleanup ProblemList-svc-vthread.txt
9d44dd0: 8297972: Poly1305 Endianness on ByteBuffer not enforced
facd415: 8297757: VarHandles.getStaticFieldFromBaseAndOffset should get the receiver type from VarHandle
e803855: 8299863: URLFromURITest.java should import org.junit.jupiter.api.Test
92d8326: 8299827: Add resolved IP address in connection exception for sockets
c6d5600: 8038146: Clarify Map.Entry's connection to the underlying map
... and 78 more: https://git.openjdk.org/jdk/compare/89a032dc057d04c996632ad317a0303cf3560852...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-21T12:00:26Z

@cl4es Pushed as commit bb42e61.

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

Use ArraysSupport.vectorizedHashCode in j.u.zip.ZipCoder

0c7f0f4

openjdk bot added the rfr Pull request is ready for review label Jan 18, 2023

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jan 18, 2023

cl4es mentioned this pull request Jan 19, 2023

8300647: Miscellaneous hashCode improvements in java.base #12093

Closed

3 tasks

AlanBateman approved these changes Jan 20, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 20, 2023

LanceAndersen reviewed Jan 20, 2023

View reviewed changes

Vary dir and entry name lengths across a wider spread, keeping most e…

369537c

…ntries short but making the longest paths longer

LanceAndersen approved these changes Jan 20, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 21, 2023

openjdk bot closed this Jan 21, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 21, 2023

liach mentioned this pull request Sep 20, 2023

8316587: Use ArraysSupport.vectorizedHashCode in Utf8EntryImpl #15837

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8300493: Use ArraysSupport.vectorizedHashCode in j.u.zip.ZipCoder #12077

8300493: Use ArraysSupport.vectorizedHashCode in j.u.zip.ZipCoder #12077

Uh oh!

cl4es commented Jan 18, 2023 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Jan 18, 2023

Uh oh!

openjdk bot commented Jan 18, 2023

Uh oh!

mlbridge bot commented Jan 18, 2023 •

edited

Loading

Uh oh!

AlanBateman commented Jan 18, 2023

Uh oh!

cl4es commented Jan 19, 2023

Uh oh!

AlanBateman commented Jan 20, 2023

Uh oh!

openjdk bot commented Jan 20, 2023 •

edited

Loading

Uh oh!

LanceAndersen Jan 20, 2023

Uh oh!

cl4es Jan 20, 2023

Uh oh!

cl4es commented Jan 20, 2023

Uh oh!

cl4es commented Jan 20, 2023

Uh oh!

LanceAndersen left a comment

Uh oh!

cl4es commented Jan 21, 2023

Uh oh!

openjdk bot commented Jan 21, 2023

Uh oh!

openjdk bot commented Jan 21, 2023

Uh oh!

Uh oh!

8300493: Use ArraysSupport.vectorizedHashCode in j.u.zip.ZipCoder #12077

8300493: Use ArraysSupport.vectorizedHashCode in j.u.zip.ZipCoder #12077

Uh oh!

Conversation

cl4es commented Jan 18, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 18, 2023

Uh oh!

openjdk bot commented Jan 18, 2023

Uh oh!

mlbridge bot commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

AlanBateman commented Jan 18, 2023

Uh oh!

cl4es commented Jan 19, 2023

Uh oh!

AlanBateman commented Jan 20, 2023

Uh oh!

openjdk bot commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LanceAndersen Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

cl4es Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

cl4es commented Jan 20, 2023

Uh oh!

cl4es commented Jan 20, 2023

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

cl4es commented Jan 21, 2023

Uh oh!

openjdk bot commented Jan 21, 2023

Uh oh!

openjdk bot commented Jan 21, 2023

Uh oh!

Uh oh!

cl4es commented Jan 18, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 18, 2023 •

edited

Loading

openjdk bot commented Jan 20, 2023 •

edited

Loading