Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8319879: Stress mode to randomize incremental inlining decision #16597

Closed
wants to merge 7 commits into from

Conversation

TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Nov 10, 2023

We had quite a few C2 bugs in the past that only reproduced with a specific sequence of incremental inlining (for example, JDK-8319764). We should randomize the incremental inlining decision to trigger these code paths more frequently.

I did some testing with the flag enabled and I hit JDK-8319764 and JDK-8319889. I think it's still valuable to integrate this even if these bugs are not resolved yet.

Thanks,
Tobias


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-8319879: Stress mode to randomize incremental inlining decision (Enhancement - P3)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16597

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 10, 2023

👋 Welcome back thartmann! 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
Copy link

openjdk bot commented Nov 10, 2023

@TobiHartmann 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.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 10, 2023
@TobiHartmann TobiHartmann marked this pull request as ready for review November 13, 2023 11:42
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 13, 2023
@mlbridge
Copy link

mlbridge bot commented Nov 13, 2023

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you switch off flag in cha/*RootMethod.java tests?

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish it was one flag - too much duplication. But I see why you separated them: to get randomness only if AlwaysIncrementalInline is not specified.
If AlwaysIncrementalInline was tuple {false | true | random} it would solve this.

@TobiHartmann
Copy link
Member Author

Why you switch off flag in cha/*RootMethod.java tests?

They already have -XX:-AlwaysIncrementalInline. If incremental inlining is used, the tests fail with:

Caused by: java.lang.RuntimeException: assertTrue: expected true, was false
	at jdk.test.lib.Asserts.fail(Asserts.java:634)
	at jdk.test.lib.Asserts.assertTrue(Asserts.java:486)
	at jdk.test.lib.Asserts.assertTrue(Asserts.java:472)
	at compiler.cha.Utils$ATest.assertNotCompiled(Utils.java:150)
	at compiler.cha.AbstractRootMethod$AbstractClass.test(AbstractRootMethod.java:132)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)

@TobiHartmann
Copy link
Member Author

Thanks for looking at this, Vladimir.

I wish it was one flag - too much duplication. But I see why you separated them: to get randomness only if AlwaysIncrementalInline is not specified. If AlwaysIncrementalInline was tuple {false | true | random} it would solve this.

Right, a tuple would be better but then again we might want to merge all these stress flags into one flag at some point. For simplicity (and backport-ability), let's keep them separated for now and do a cleanup later. There's more to come, especially in the area of loop opts.

@@ -185,7 +185,7 @@ CallGenerator* Compile::call_generator(ciMethod* callee, int vtable_index, bool
// Try inlining a bytecoded method:
if (!call_does_dispatch) {
InlineTree* ilt = InlineTree::find_subtree_from_root(this->ilt(), jvms->caller(), jvms->method());
bool should_delay = AlwaysIncrementalInline;
bool should_delay = AlwaysIncrementalInline || (StressIncrementalInlining && (C->random() % 2) == 0);
Copy link
Member

@jatin-bhateja jatin-bhateja Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason to keep vector, string , boxing call generators out of scope, I understand that lazy intrinsification gives constants and opportunity to seep in the graph and enable meeting closed world compilation constraints, but this is a stress testing option, an eager failed intrinsification will take the control flow to line# 214 , but only if we randomize creating LateInlineCallGenerators for these APIs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by keeping them out of scope? String, boxing and vector methods will be late inlined if the respective should_delay_*_inlining method returns true. Otherwise, the stress option might still trigger late inlining randomly. Isn't that exactly what we want? Or do you suggest to check should_delay first?

Copy link
Member

@jatin-bhateja jatin-bhateja Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, should_delay_vector_inlining is not affected by newly added option. Yes, should_delay check should be earlier if we intend to randomize lazy intrinsification decisions for vector , boxing methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I think that would be reasonable. I'll give that a try.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work fine. I moved the check up.

@vnkozlov
Copy link
Contributor

Why you switch off flag in cha/*RootMethod.java tests?

They already have -XX:-AlwaysIncrementalInline. If incremental inlining is used, the tests fail with:

Caused by: java.lang.RuntimeException: assertTrue: expected true, was false
	at jdk.test.lib.Asserts.fail(Asserts.java:634)
	at jdk.test.lib.Asserts.assertTrue(Asserts.java:486)
	at jdk.test.lib.Asserts.assertTrue(Asserts.java:472)
	at compiler.cha.Utils$ATest.assertNotCompiled(Utils.java:150)
	at compiler.cha.AbstractRootMethod$AbstractClass.test(AbstractRootMethod.java:132)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)

I don't see -XX:-AlwaysIncrementalInline in these 2 tests. They have only -XX:-StressMethodHandleLinkerInlining which only control inlining for MH. I am not saying that adding -XX:-StressIncrementalInlining is wrong but I need explanation.

@vnkozlov
Copy link
Contributor

vnkozlov commented Nov 14, 2023

Thanks for looking at this, Vladimir.

I wish it was one flag - too much duplication. But I see why you separated them: to get randomness only if AlwaysIncrementalInline is not specified. If AlwaysIncrementalInline was tuple {false | true | random} it would solve this.

Right, a tuple would be better but then again we might want to merge all these stress flags into one flag at some point. For simplicity (and backport-ability), let's keep them separated for now and do a cleanup later. There's more to come, especially in the area of loop opts.

Can we change meaning of -XX:+AlwaysIncrementalInline -XX:+StressIncrementalInlining combination?
Currently with -XX:+AlwaysIncrementalInline we ignore StressIncrementalInlining setting.
What if -XX:+StressIncrementalInlining enables -XX:+AlwaysIncrementalInline but make it random (my tuple idea).
In such case you don't need to check StressIncrementalInlining where you check AlwaysIncrementalInline.
But the randomizing condition have to be change:

  bool should_delay = AlwaysIncrementalInline && (!StressIncrementalInlining || (C->random() % 2) == 0);

Also -XX:-AlwaysIncrementalInline will disable StressIncrementalInlining so you don't need to update tests.

@TobiHartmann
Copy link
Member Author

I don't see -XX:-AlwaysIncrementalInline in these 2 tests. They have only -XX:-StressMethodHandleLinkerInlining which only control inlining for MH. I am not saying that adding -XX:-StressIncrementalInlining is wrong but I need explanation.

You are right, I looked at the wrong test. AbstractRootMethod and DefaultRootMethod failed in my testing but looking at it again, they also intermittently failed with -XX:+AlwaysIncrementalInline. I think that might be because I added should_delay to CallGenerator::for_method_handle_call. I'll check again with Roland's fix for JDK-8319764 because these failures generated a lot of noise.

Can we change meaning of -XX:+AlwaysIncrementalInline -XX:+StressIncrementalInlining combination?

AlwaysIncrementalInline is a debug flag and therefore always false in product, whereas StressIncrementalInlining, similar to the other stress flags that we have, is a diagnostic flag. Making it dependent on AlwaysIncrementalInline will essentially disable it in product.

Also, I think it would be misleading if AlwaysIncrementalInlining would not always lead to incremental inlining, right?

Separately, we could convert AlwaysIncrementalInlining to a compiler directive to enable it on a per-method basis. Because in all the use cases that I can imagine, you usually want to enable it just for a few methods and not for all. And if you want stress testing, you can now use -XX:+StressIncrementalInlining which is even more powerful.

@vnkozlov
Copy link
Contributor

AlwaysIncrementalInline is a debug flag and therefore always false in product, whereas StressIncrementalInlining, similar to the other stress flags that we have, is a diagnostic flag. Making it dependent on AlwaysIncrementalInline will essentially disable it in product.

Okay.

@TobiHartmann
Copy link
Member Author

I'll check again with Roland's fix for JDK-8319764 because these failures generated a lot of noise.

It seems that these test changes were not required so I removed them.

@TobiHartmann
Copy link
Member Author

TobiHartmann commented Nov 17, 2023

I executed a lot of testing up to and including tier7 + stress testing with -XX:+StressIncrementalInlining and found several bugs that I linked to JDK-8319879. It turned out that the compiler/cha/*tests do fail but the failure is intermittent. I filed an issue for that as well.

I think that proves the value of this stress flag and given that these bugs are independent of the stress flag implementation, I propose to integrate it even if these issues are still unresolved.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

@openjdk
Copy link

openjdk bot commented Nov 17, 2023

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

8319879: Stress mode to randomize incremental inlining decision

Reviewed-by: kvn, roland

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

  • a6098e4: 8320280: RISC-V: Avoid passing t0 as temp register to MacroAssembler::lightweight_lock/unlock
  • 60c8d9c: 8320300: Adjust hs_err output in malloc/mmap error cases
  • 9f6a4a3: 8320386: Remove java/nio/channels/vthread/BlockingChannelOps.java#direct-register from ProblemList
  • db1d823: 8320168: handle setsocktopt return values
  • 6c5e15c: 8316557: Make fields final in 'sun.util' package
  • de51aa1: 8283214: [macos] Screen magnifier does not show the magnified text for JcomboBox
  • 179f505: 8320361: Doc error in RemoteRecordingStream.java
  • 8ff7d6e: 8295391: Add discussion of binary <-> decimal conversion issues
  • 99570fb: 8320258: Refresh building.md
  • a1e7a30: 8320212: Disable GCC stringop-overflow warning for affected files
  • ... and 89 more: https://git.openjdk.org/jdk/compare/6b21ff61dad6f633c744c1c33c29ea86183b509d...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 openjdk bot added the ready Pull request is ready to be integrated label Nov 17, 2023
@TobiHartmann
Copy link
Member Author

Thanks, Vladimir.

Copy link
Contributor

@rwestrel rwestrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@@ -989,8 +989,9 @@ CallGenerator* CallGenerator::for_method_handle_call(JVMState* jvms, ciMethod* c
bool input_not_const;
CallGenerator* cg = CallGenerator::for_method_handle_inline(jvms, caller, callee, allow_inline, input_not_const);
Compile* C = Compile::current();
bool should_delay = AlwaysIncrementalInline || (StressIncrementalInlining && (C->random() % 2) == 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should go in its own function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, Roland! I added a new should_delay_inlining method to Compile.

@TobiHartmann
Copy link
Member Author

Thanks again, Roland!

@TobiHartmann
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Nov 21, 2023

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

  • 099a8f5: 8308144: Uncontrolled memory consumption in SSLFlowDelegate.Reader
  • fcb4df2: 8320192: SHAKE256 does not work correctly if n >= 137
  • 2b4e991: 8320208: Update Public Suffix List to b5bf572
  • 6b96bb6: 8319777: Zero: Support 8-byte cmpxchg
  • 020c900: 8320052: Zero: Use __atomic built-ins for atomic RMW operations
  • 30d8953: 8275889: Search dialog has redundant scrollbars
  • cee54de: 8319988: Wrong heading for inherited nested classes
  • 32098ce: 8320348: test/jdk/java/io/File/GetAbsolutePath.windowsDriveRelative fails if working directory is not on drive C
  • a2c0fa6: 8320372: test/jdk/sun/security/x509/DNSName/LeadingPeriod.java validity check failed
  • 3aefd1c: 8320234: Merge doclint.Env.AccessKind with tool.AccessKind
  • ... and 104 more: https://git.openjdk.org/jdk/compare/6b21ff61dad6f633c744c1c33c29ea86183b509d...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 21, 2023
@openjdk openjdk bot closed this Nov 21, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 21, 2023
@openjdk
Copy link

openjdk bot commented Nov 21, 2023

@TobiHartmann Pushed as commit 303757b.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants