8316587: Use ArraysSupport.vectorizedHashCode in Utf8EntryImpl by liach · Pull Request #15837 · openjdk/jdk

liach · 2023-09-20T09:17:24Z

Like #12077, this uses JDK's internal utilities to speed up ASCII reading in Class files, where most identifiers, from conventions to attribute names, are ASCII. See the JBS issue for more in-depth explanations.

Before: (Master)

Benchmark                         Mode  Cnt    Score   Error  Units
ReadMetadata.jdkReadMemberNames  thrpt    4  167.623 ± 8.522  ops/s

After: (This patch, first revision)

Benchmark                         Mode  Cnt    Score   Error  Units
ReadMetadata.jdkReadMemberNames  thrpt    4  175.908 ± 4.766  ops/s

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-8316587: Use ArraysSupport.vectorizedHashCode in Utf8EntryImpl (Enhancement - P4)

Reviewers

Adam Sotona (@asotona - Reviewer)
Claes Redestad (@cl4es - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15837

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-20T09:18:48Z

👋 Welcome back liach! 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-09-20T09:21:50Z

@liach 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 · 2023-09-20T09:25:55Z

Webrevs

01: Full - Incremental (6f5e3d72)
00: Full (d0ce1181)

liach · 2023-09-21T09:33:07Z

@asotona Since this is your area, can you take a look?
@cl4es Can you review this as well since you are more professional in string optimizations, and can you see if you have similar performance improvements?

cl4es · 2023-09-21T12:18:26Z

I can verify the improvement locally in your added micro:

Name                            Cnt         Base    Error          Test    Error   Unit   Diff%
ReadMetadata.jdkReadMemberNames   4      108.234 ±  1.089       114.953 ±  0.709  ops/s    6.2% (p = 0.000*)

I don't have domain expertise w.r.t the classfile API to value how much this will benefit typical use-cases, but it seem like a straightforward enough improvement to me. (At least the vectorizedHashCode has the nice property that it's always faster than a naive 31 * hash + v loop, even when inputs are too small to actually benefit from SIMD instructions).

asotona · 2023-09-21T13:43:30Z

src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java

-
-            if (!foundHigh) {
+            int singleBytes = JLA.countPositives(rawBytes, offset, rawLen);
+            int hash = ArraysSupport.vectorizedHashCode(rawBytes, offset, singleBytes, 0, ArraysSupport.T_BOOLEAN);


We should keep in mind that this code is on lambdas bootstrap critical path.
Any foreign code may involve lambdas or method references and cause dead end in lambdas initialization after JDK-8294960.

This is safe: these two functions are already used in String itself (see StringLatin1) which is definitely loaded and initialized before Classfile API and its implementations are.

Agreed w.r.t bootstrap safety: countPositives and vectorizedHashCode are straightforward java routines used by String and others very early in the bootstrap sequence. The speed-up they bring come from being intrinsified by the JIT.

openjdk · 2023-09-21T19:13:50Z

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

8316587: Use ArraysSupport.vectorizedHashCode in Utf8EntryImpl

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

a1e0346: 8316698: build failure caused by JDK-8316456
c72f004: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly
041510d: 8315486: vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn002/forceEarlyReturn002.java timed out
ef49e6c: 8316629: j.text.DateFormatSymbols setZoneStrings() exception is unhelpful
496264c: 8316435: sun.util.calendar.CalendarSystem subclassing should be restricted
1100dbc: 8316695: ProblemList serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java
d3e8218: 8316306: Open source and convert manual Swing test
4e57177: 8316383: NullPointerException in AbstractSAXParser after JDK-8306632
3b397c8: 8315965: Open source various AWT applet tests
c698b45: 8313229: DHEKeySizing.java should be modified to use TLS versions TLSv1, TLSv1.1, TLSv1.2
... and 37 more: https://git.openjdk.org/jdk/compare/ec74194cb75afcaab2f77e8728391bb9104ccc73...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@asotona, @cl4es) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

liach · 2023-09-22T00:21:21Z

/integrate

openjdk · 2023-09-22T00:22:16Z

@liach
Your change (at version 6f5e3d7) is now ready to be sponsored by a Committer.

bridgekeeper · 2023-10-20T06:58:51Z

@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

liach · 2023-10-20T07:03:56Z

Can anyone sponsor this patch? This should be compatible with Class-File transition to preview as this does not change any API.

asotona · 2023-10-20T07:34:18Z

/sponsor

openjdk · 2023-10-20T07:36:31Z

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

bd02cfd: 8318540: make test cannot run .jasm tests directly
bd3bc2c: 8317350: Move code cache purging out of CodeCache::UnloadingScope
292aad2: 8316436: ContinuationWrapper uses unhandled nullptr oop
387504c: 8317575: AArch64: C2_MacroAssembler::fast_lock uses rscratch1 for cmpxchg result
d9ce525: 8318150: StaticProxySelector.select should not throw NullPointerExceptions
c46a54e: 8312777: notifyJvmtiMount before notifyJvmtiUnmount
8f5f440: 8317692: jcmd GC.heap_dump performance regression after JDK-8292818
684b91e: 8315064: j.text.ChoiceFormat provides no specification on quoting behavior
1740950: 8314901: AES-GCM interleaved implementation using AVX2 instructions
cc8f8da: 8318322: Update IANA Language Subtag Registry to Version 2023-10-16
... and 386 more: https://git.openjdk.org/jdk/compare/ec74194cb75afcaab2f77e8728391bb9104ccc73...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-20T07:36:42Z

@asotona @liach Pushed as commit 4812cab.

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

Use JLA string utilities for utf8 entry reading

d0ce118

openjdk bot added the rfr Pull request is ready for review label Sep 20, 2023

openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 20, 2023

Fix logical bug with char array filling

6f5e3d7

asotona reviewed Sep 21, 2023

View reviewed changes

asotona approved these changes Sep 21, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 21, 2023

cl4es approved these changes Sep 21, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 22, 2023

openjdk bot added the integrated Pull request has been integrated label Oct 20, 2023

openjdk bot closed this Oct 20, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 20, 2023

Conversation

liach commented Sep 20, 2023 • 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 20, 2023

Uh oh!

openjdk bot commented Sep 20, 2023

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

liach commented Sep 21, 2023

Uh oh!

cl4es commented Sep 21, 2023

Uh oh!

asotona Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

liach Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

cl4es Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

liach commented Sep 22, 2023

Uh oh!

openjdk bot commented Sep 22, 2023

Uh oh!

bridgekeeper bot commented Oct 20, 2023

Uh oh!

liach commented Oct 20, 2023

Uh oh!

asotona commented Oct 20, 2023

Uh oh!

openjdk bot commented Oct 20, 2023

Uh oh!

openjdk bot commented Oct 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

liach commented Sep 20, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 20, 2023 •

edited

Loading

openjdk bot commented Sep 21, 2023 •

edited

Loading