8320448: Accelerate IndexOf using AVX2 #16753

asgibbons · 2023-11-21T00:06:19Z

Re-write the IndexOf code without the use of the pcmpestri instruction, only using AVX2 instructions. This change accelerates String.IndexOf on average 1.3x for AVX2. The benchmark numbers:

Benchmark	                                               Score		Latest		
StringIndexOf.advancedWithMediumSub   343.573		317.934		0.925375393x
StringIndexOf.advancedWithShortSub1	  1039.081		1053.96		1.014319384x
StringIndexOf.advancedWithShortSub2	      55.828		110.541		1.980027943x
StringIndexOf.constantPattern	                9.361		11.906		1.271872663x
StringIndexOf.searchCharLongSuccess	        4.216		4.218		1.000474383x
StringIndexOf.searchCharMediumSuccess	3.133		3.216		1.02649218x
StringIndexOf.searchCharShortSuccess	3.76		        3.761		1.000265957x
StringIndexOf.success	                                9.186		9.713		1.057369911x
StringIndexOf.successBig	                      14.341		46.343		3.231504079x
StringIndexOfChar.latin1_AVX2_String	  6220.918		12154.52		1.953814533x
StringIndexOfChar.latin1_AVX2_char	  5503.556		5540.044		1.006629895x
StringIndexOfChar.latin1_SSE4_String	  6978.854		6818.689		0.977049957x
StringIndexOfChar.latin1_SSE4_char	  5657.499		5474.624		0.967675646x
StringIndexOfChar.latin1_Short_String	  7132.541		6863.359		0.962260014x
StringIndexOfChar.latin1_Short_char	16013.389	      16162.437		1.009307711x
StringIndexOfChar.latin1_mixed_String	  7386.123	      14771.622		1.999915517x
StringIndexOfChar.latin1_mixed_char	  9901.671		9782.245		0.987938803

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (3 reviews required, with at least 3 Reviewers)

Issue

JDK-8320448: Accelerate IndexOf using AVX2 (Enhancement - P4)

Reviewers

Emanuel Peter (@eme64 - Reviewer) ⚠️ Review applies to 6eae46e5
Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to ed06edd6
Sandhya Viswanathan (@sviswa7 - Reviewer) ⚠️ Review applies to db0ab75a

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16753

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-21T00:07:09Z

👋 Welcome back sgibbons! 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-11-21T00:09:40Z

@asgibbons this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout indexof
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2023-11-21T00:10:07Z

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

core-libs
hotspot

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.

asgibbons · 2023-11-29T01:06:59Z

Opening up for review. Fixed last whitespace error. I will post final performance numbers soon.

mlbridge · 2023-11-29T01:11:20Z

Webrevs

asgibbons · 2023-11-29T18:34:41Z

Latest numbers vs. top-of-tree JDK.

asgibbons · 2024-05-30T14:57:35Z

@eme64 I'm glad to have received your feedback. I see I have erroneously assumed that by making the exact code change you requested still requires your acceptance - I won't make that mistake again. I had also erroneously assumed that your review was complete and you had no further changes for me to make. I'd also not like to make that mistake again, but I'm unsure how to conclude that a review is complete - it seems like 7 hours of elapsed time isn't sufficient to indicate completion, so can you please help me figure this out? Perhaps it's just my distaste for "trickle-in" comments, which I should get over, or is there another way you can suggest?

As for the fuzzer I would be very interested in learning more about this. We have a significant number of compute resources, so it may be valuable for us to set up a copy of the fuzzer on-site to improve the quality of our submissions. Can you help in pointing me to someone that can advise me on how to do this?

As for holding off the integration, I'll leave the decision to a sponsor for this PR. I don't believe increasing the reviewer count just to "force" reevaluation should be an acceptable practice, although I'm not an insider in this community.

vpaprotsk · 2024-05-30T15:13:22Z

@eme64 I guess to add some confidence.. we did also 'test it independently' to catch blind spots. i.e. String/IndexOf.java is from me. I tried to be as paranoid as possible with non-random strings. Passed everything I could throw at it..

eme64 · 2024-05-30T15:15:04Z

@asgibbons I was done with my review, or at least so I thought 😉
Still: if I give comments, it would be nice to quickly finish the conversation, unless if I don't respond in many days and not even to emails.

Often I only see the glaring issues. Then you fix them, and then I see something else around it. Then I may give more comments. That is what happened.

If I think that I have small suggestions and then I'm done, then I might even approve even though there are suggestions still to be added.

I just put up the limit really quick so that nobody else would by accident sponsor it before we have finished the conversation, and I will definitely give you my approval once the little issues are resolved ;)

eme64 · 2024-05-30T15:16:34Z

About the fuzzer: we have it in our closed tests. But I think it comes from this: https://github.com/shipilev/JavaFuzzer

eme64 · 2024-05-30T15:33:16Z

test/jdk/java/lang/StringBuffer/ECoreIndexOf.java

+ */
+
+/* @test
+ * @bug 4162796 4162796 8320448


Suggested change

* @bug 4162796 4162796 8320448

* @bug 8320448

As I said above: I never add old bug numbers to new tests. But here it is even duplicated ;)

The file I used as baseline for this test/jdk/java/lang/StringBuffer/IndexOf.java has the bug number listed twice (copy/paste). I'll remove it from here, but leave it in the original unless requested to change it.

eme64 · 2024-05-30T15:33:27Z

test/jdk/java/lang/StringBuffer/ECoreIndexOf.java

+ */
+
+/* @test
+ * @bug 4162796 4162796 8320448


Suggested change

* @bug 4162796 4162796 8320448

* @bug 8320448

eme64

Ok, now it is good for me. But I would definately wait with integration for after the fork next week.

eme64 · 2024-05-30T16:03:29Z

src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp

@@ -0,0 +1,1837 @@
+/*
+ * Copyright (c) 2023, 2024 Intel Corporation. All rights reserved.


Is the 2023 year intentional? I don't know your policy, so you can just ignore this ;)

I started this in November :-)

eme64 · 2024-05-30T16:04:34Z

src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp

+  // and <= 32 bytes for small.  The switches implement optimized code for handling 1 to
+  // NUMBER_OF_CASES (currently 10) needle sizes for both big and small.  There are special
+  // routines for handling needle sizes > NUMBER_OF_CASES (L_{big,small}CaseDefault).  These
+  // cases use C@'s arrays_equals() to compare the needle to the haystack.  The small cases


Suggested change

// cases use C@'s arrays_equals() to compare the needle to the haystack. The small cases

// cases use C2's arrays_equals() to compare the needle to the haystack. The small cases

Randomly spotted this.

eme64 · 2024-05-30T16:06:17Z

src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp

+    // cmp nndx, ndlLen
+    // jae done
+    //
+    // Final index of start of needle @((16 - (ndlLen %16)) & 0xf) << 1


What is the meaning of the @? Maybe at. I'd use the same consistently

Changed to "at".

vnkozlov · 2024-05-30T16:10:53Z

I agree with @eme64 to postpone the integration after JDK 23 is forked in one week. It is not about how you confident with code. It is size of code. I did only limited (tier1-4) testing in our infra which did not cover all our testing configuration.

asgibbons · 2024-05-30T16:16:59Z

@vnkozlov OK. I'll defer to you all. I've contacted the author of the fuzzer to see what I can do to set up a local instance. Would this be sufficient to increase confidence for future submissions? We can run it perpetually on fixes (provided I can set it up). Had I done that, we could have had 6 months of fuzzing on top of our tests. Would that have alleviated this concern?

eme64 · 2024-05-30T16:20:02Z

@asgibbons I generally just stop pushing ANY RFE's a week or two before the fork. Even if you did run the fuzzer with it - there are often last-minute changes. And your code here is rather large, so even if you are confident, there must be at least one bug hiding.

Running the fuzzer is nice as pre-integration, but it mostly only catches things post-integration.

asgibbons · 2024-06-06T17:41:20Z

Hi, everyone. I see that JDK 23 has now been forked, and new commits go into the JDK 24 branch. I would like to get this in as soon as possible to have as much time with fuzzers, etc. for everyone to be confident in the code.

I have 3 positive reviews on this PR and would like to integrate. Please reply as soon as you reasonably can with objections or approval and I will integrate. Thanks.

vnkozlov · 2024-06-06T18:25:35Z

Let me do quick testing with latest mainline (JDK 24 now).

vnkozlov · 2024-06-06T20:37:43Z

@asgibbons, my testing almost finished. No new failures. I think this can be pushed now. Thank you for waiting!

asgibbons · 2024-06-06T21:44:44Z

@eme64 Are you OK with me integrating?

eme64 · 2024-06-07T05:05:27Z

@asgibbons yes, ship it! 🚢
Thanks for waiting!

asgibbons · 2024-06-07T15:23:05Z

/integrate

openjdk · 2024-06-07T15:24:11Z

@asgibbons
Your change (at version f432320) is now ready to be sponsored by a Committer.

jatin-bhateja · 2024-06-07T17:01:14Z

/sponsor

openjdk · 2024-06-07T17:02:18Z

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

25ad862: 8332161: Test restoring echo in the Console implementation (java.base)
ee82346: 8325984: 4 jcstress tests are failing in Tier6 4 times each
d744059: 8333774: Avoid eagerly loading various EmptySpliterator classes
d130d2f: 8333477: Delete extra empty spaces in Makefiles
486dee2: 8333653: Remove MallocHeader::get_stack
40b2fbd: 8331733: [PPC64] saving and restoring CR is not needed at most places
6968770: 8331935: Add support for primitive array C1 clone intrinsic in PPC
a2030ff: 8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs
bf7f1c4: 8333211: NMT Reports: replace manual indentation handling with auto indent
8ffc35d: 8333724: Problem list security/infra/java/security/cert/CertPathValidator/certification/CAInterop.java#teliasonerarootcav1
... and 255 more: https://git.openjdk.org/jdk/compare/37c477856d543163b60dd2b85a5e6ac35a752211...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-07T17:02:24Z

@jatin-bhateja @asgibbons Pushed as commit 8e72d7c.

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

asgibbons added 8 commits August 23, 2023 18:59

sync push

d40a162

Adding string assembly - first cut

e33721a

Removed comments; only generate for AVX2

356cd64

Infrastructure hookup

139151f

Fix single-character; make Windows compile

30f1c08

Fix jmp table; optimize length 1

4d68677

Protecting against page faults

365c257

Working version

58ffe6f

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 21, 2023

openjdk bot added hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Nov 21, 2023

Merge branch 'master' into indexof

ce38002

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 21, 2023

asgibbons added 7 commits November 20, 2023 16:37

Fix merge problem

3c2df63

Windows register preservation fix

c01ff14

Stomped on r13 in switch branch calculation

6882bac

Subtracting 0x10 twice.

dbe6a35

Comments; added exhaustive-ish test

8464376

Merge branch 'openjdk:master' into indexof

db3e15f

Fix whitespace

60d762b

asgibbons marked this pull request as ready for review November 29, 2023 01:06

openjdk bot added the rfr Pull request is ready for review label Nov 29, 2023

Only use optimization when EnableX86ECoreOpts is true

e614b86

Support UU IndexOf

5e03173

Review comments

57e115d

eme64 reviewed May 30, 2024

View reviewed changes

Fix bug number in tests

6eae46e

eme64 approved these changes May 30, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 30, 2024

Fix copyright & a couple of comment typos

f432320

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 7, 2024

openjdk bot added the integrated Pull request has been integrated label Jun 7, 2024

openjdk bot closed this Jun 7, 2024

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 Jun 7, 2024

robcasloz mentioned this pull request Sep 26, 2024

8305895: Implement JEP 450: Compact Object Headers (Experimental) #20677

Closed

21 tasks

		@@ -0,0 +1,1837 @@
		/*
		* Copyright (c) 2023, 2024 Intel Corporation. All rights reserved.

	// cases use C@'s arrays_equals() to compare the needle to the haystack. The small cases
	// cases use C2's arrays_equals() to compare the needle to the haystack. The small cases

8320448: Accelerate IndexOf using AVX2 #16753

8320448: Accelerate IndexOf using AVX2 #16753

Uh oh!

Conversation

asgibbons commented Nov 21, 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 Nov 21, 2023

Uh oh!

openjdk bot commented Nov 21, 2023

Uh oh!

openjdk bot commented Nov 21, 2023

Uh oh!

asgibbons commented Nov 29, 2023

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

asgibbons commented Nov 29, 2023

Uh oh!

asgibbons commented May 30, 2024

Uh oh!

vpaprotsk commented May 30, 2024

Uh oh!

eme64 commented May 30, 2024

Uh oh!

eme64 commented May 30, 2024

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!

eme64 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!

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!

vnkozlov commented May 30, 2024

Uh oh!

asgibbons commented May 30, 2024

Uh oh!

eme64 commented May 30, 2024

Uh oh!

asgibbons commented Jun 6, 2024

Uh oh!

vnkozlov commented Jun 6, 2024

Uh oh!

vnkozlov commented Jun 6, 2024

Uh oh!

asgibbons commented Jun 6, 2024

Uh oh!

eme64 commented Jun 7, 2024

Uh oh!

asgibbons commented Jun 7, 2024

Uh oh!

openjdk bot commented Jun 7, 2024

Uh oh!

jatin-bhateja commented Jun 7, 2024

asgibbons commented Nov 21, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 29, 2023 •

edited

Loading