8273585: String.charAt performance degrades due to JDK-8268698 #6096

y1yang0 · 2021-10-25T05:52:02Z

String.charAt shows significant performance regression due to JDK-8268698, which replaces index bound checking with Preconditions.checkIndex intrinsic method.

The result of "time linux-x86_64-server-release/images/jdk/bin/java Test":

Before JDK-8268698
    real 0m8.369s
    user 0m8.386s
    sys 0m0.019s

After JDK-8268698,
    real 0m19.722s
    user 0m19.748s
    sys 0m0.013s

The reason is Preconditions.checkIndex generates a CastII for index node as index is now known to be >= 0 and < length.:

jdk/src/hotspot/share/opto/library_call.cpp

Lines 1077 to 1083 in 5dab76b

    
           // index is now known to be >= 0 and < length, cast it 
        
           Node* result = ConstraintCastNode::make(control(), index, TypeInteger::make(0, upper_bound, Type::WidenMax, bt), bt); 
        
           result = _gvn.transform(result); 
        
           set_result(result); 
        
           replace_in_map(index, result); 
        
           clear_upper_avx(); 
        
           return true;

CastII can not be recognized as a parallel induction variable because AddNode's input must be the PhiNode:

jdk/src/hotspot/share/opto/loopnode.cpp

Lines 3177 to 3184 in 5dab76b

    
           // Look for induction variables of the form:  X += constant 
        
           if (phi2->region() != loop->_head || 
        
               incr2->req() != 3 || 
        
               incr2->in(1) != phi2 || 
        
               incr2 == incr || 
        
               incr2->Opcode() != Op_AddI || 
        
               !incr2->in(2)->is_Con()) 
        
             continue;

It seems this prevents further loop unrolling. I think we can relax this constraint, i.e CastII can be the input of AddNode if its input is PhiNode. After applying this patch, performance regression disappears:

$time ./test.sh 

real    0m9.514s
user    0m10.310s
sys     0m0.155s

This is likely the reason for JDK-8272493. Please help review it. Thanks!

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8273585: String.charAt performance degrades due to JDK-8268698

Reviewers

Roland Westrelin (@rwestrel - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6096/head:pull/6096
$ git checkout pull/6096

Update a local copy of the PR:
$ git checkout pull/6096
$ git pull https://git.openjdk.java.net/jdk pull/6096/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6096

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6096.diff

bridgekeeper · 2021-10-25T05:52:29Z

👋 Welcome back yyang! 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 · 2021-10-25T05:54:51Z

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

hotspot-compiler

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 · 2021-10-25T05:59:16Z

Webrevs

01: Full - Incremental (992906df)
00: Full (8029f673)

rwestrel · 2021-10-25T12:27:42Z

src/hotspot/share/opto/loopnode.cpp

        incr2 == incr ||
        incr2->Opcode() != Op_AddI ||
        !incr2->in(2)->is_Con())
      continue;
-
+    if (incr2->in(1) != phi2) {


You could use incr2->in(1)->uncast() != phi2. You don't need to add an extra if then.

Since the semantic of uncast is enough clear, I also remove the comment.

rwestrel

Looks good to me.

openjdk · 2021-10-26T11:46:12Z

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

8273585: String.charAt performance degrades due to JDK-8268698

Reviewed-by: roland, kvn

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

f1f5e26: 8275872: Sync J2DBench run and analyze Makefile targets with build.xml
19f76c2: 8275079: Remove unnecessary conversion to String in java.net.http
e5cd269: 8274944: AppCDS dump causes SEGV in VM thread while adjusting lambda proxy class info
82f4aac: 8259609: C2: optimize long range checks in long counted loops
574f890: 8275720: CommonComponentAccessibility.createWithParent isWrapped causes mem leak
7c88a59: 8275809: crash in [CommonComponentAccessibility getCAccessible:withEnv:]
c9dec2f: 8273299: Unnecessary Vector usage in java.security.jgss
b98ed55: 8275819: [TableRowAccessibility accessibilityChildren] method is ineffective
71d593e: 8275162: Use varargs in 'def' macros in mutexLocker.cpp
7ca053d: 8251904: vmTestbase/nsk/sysdict/vm/stress/btree/btree010/btree010.java fails with ClassNotFoundException: nsk.sysdict.share.BTree0LLRLRLRRLR
... and 161 more: https://git.openjdk.java.net/jdk/compare/b8cb76ad210cb3e7524c7f5b13cfe57746ac05d4...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.

vnkozlov

Good.

cl4es · 2021-10-27T00:33:37Z

This is likely the reason for JDK-8272493.

While a good fix to an issue that seems more likely to be of real concern, this does not seem to remedy the comparatively minor performance issue reported by JDK-8272493

y1yang0 · 2021-10-27T01:19:54Z

@cl4es Thanks for confirmation. I'd like to investigate JDK-8272493 later to see what's the real cause for that one.

y1yang0 · 2021-10-27T01:20:12Z

/integrate

openjdk · 2021-10-27T01:21:23Z

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

7addcd7: 8276034: ProblemList gtest dll_address_to_function_and_library_name on macosx-x64
2448b3f: 8275874: [JVMCI] only support aligned reads in c2v_readFieldValue
f1f5e26: 8275872: Sync J2DBench run and analyze Makefile targets with build.xml
19f76c2: 8275079: Remove unnecessary conversion to String in java.net.http
e5cd269: 8274944: AppCDS dump causes SEGV in VM thread while adjusting lambda proxy class info
82f4aac: 8259609: C2: optimize long range checks in long counted loops
574f890: 8275720: CommonComponentAccessibility.createWithParent isWrapped causes mem leak
7c88a59: 8275809: crash in [CommonComponentAccessibility getCAccessible:withEnv:]
c9dec2f: 8273299: Unnecessary Vector usage in java.security.jgss
b98ed55: 8275819: [TableRowAccessibility accessibilityChildren] method is ineffective
... and 163 more: https://git.openjdk.java.net/jdk/compare/b8cb76ad210cb3e7524c7f5b13cfe57746ac05d4...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-27T01:21:38Z

@kelthuzadx Pushed as commit b0d1e4f.

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

8273585: String.charAt performance degrades due to JDK-8268698

8029f67

openjdk bot added the rfr Pull request is ready for review label Oct 25, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 25, 2021

rwestrel suggested changes Oct 25, 2021

View reviewed changes

use uncast

992906d

rwestrel approved these changes Oct 26, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 26, 2021

vnkozlov approved these changes Oct 26, 2021

View reviewed changes

openjdk bot closed this Oct 27, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 27, 2021

y1yang0 deleted the ra branch October 27, 2021 01:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8273585: String.charAt performance degrades due to JDK-8268698 #6096

8273585: String.charAt performance degrades due to JDK-8268698 #6096

y1yang0 commented Oct 25, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 25, 2021

openjdk bot commented Oct 25, 2021

mlbridge bot commented Oct 25, 2021 •

edited

Loading

rwestrel Oct 25, 2021

y1yang0 Oct 26, 2021

rwestrel left a comment

openjdk bot commented Oct 26, 2021 •

edited

Loading

vnkozlov left a comment

cl4es commented Oct 27, 2021

y1yang0 commented Oct 27, 2021

y1yang0 commented Oct 27, 2021

openjdk bot commented Oct 27, 2021

openjdk bot commented Oct 27, 2021

	// index is now known to be >= 0 and < length, cast it
	Node* result = ConstraintCastNode::make(control(), index, TypeInteger::make(0, upper_bound, Type::WidenMax, bt), bt);
	result = _gvn.transform(result);
	set_result(result);
	replace_in_map(index, result);
	clear_upper_avx();
	return true;

	// Look for induction variables of the form: X += constant
	if (phi2->region() != loop->_head \|\|
	incr2->req() != 3 \|\|
	incr2->in(1) != phi2 \|\|
	incr2 == incr \|\|
	incr2->Opcode() != Op_AddI \|\|
	!incr2->in(2)->is_Con())
	continue;

8273585: String.charAt performance degrades due to JDK-8268698 #6096

8273585: String.charAt performance degrades due to JDK-8268698 #6096

Conversation

y1yang0 commented Oct 25, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 25, 2021

openjdk bot commented Oct 25, 2021

mlbridge bot commented Oct 25, 2021 • edited Loading

Webrevs

rwestrel Oct 25, 2021

Choose a reason for hiding this comment

y1yang0 Oct 26, 2021

Choose a reason for hiding this comment

rwestrel left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 26, 2021 • edited Loading

vnkozlov left a comment

Choose a reason for hiding this comment

cl4es commented Oct 27, 2021

y1yang0 commented Oct 27, 2021

y1yang0 commented Oct 27, 2021

openjdk bot commented Oct 27, 2021

openjdk bot commented Oct 27, 2021

y1yang0 commented Oct 25, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 25, 2021 •

edited

Loading

openjdk bot commented Oct 26, 2021 •

edited

Loading