8335334: Stress mode to randomly execute unstable if traps #21037

TobiHartmann · 2024-09-17T11:01:27Z

Unstable if traps are supposed to be taken rarely. This patch introduces a StressUnstableIfTraps flag that forces unstable if traps to be taken randomly and thus potentially triggering intermittent bugs such as incorrect debug information. It works by adding another if before the unstable if that checks a "random" condition at runtime (a simple shared counter) and then either takes the trap or executes the original, unstable if.

This stress option also has the nice side effect of triggering re-compilation of methods that would otherwise not be re-compiled (see JDK-8335843).

I had to adjust a few tests that rely on methods being compiled / deoptimized because with the stress option enabled, deoptimization might unexpectedly (not) happen.

It reliably triggers JDK-8335977, JDK-8320308 and JDK-8335843 in our testing.

Tested with multiple runs up to tier6. I'll integrate it into our (Oracle internal) testing and CTW (JDK-8340302) once all the bugs that it currently triggers are fixed.

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-8335334: Stress mode to randomly execute unstable if traps (Enhancement - P4)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) Review applies to 691af16c
Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21037

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-17T11:02:04Z

👋 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 · 2024-09-17T11:03:33Z

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

8335334: Stress mode to randomly execute unstable if traps

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

384deda: 8325949: Create an internal utility method for creating VarHandle instances
67448b0: 8339852: Fix typos in java.compiler documentation
bc7c0dc: 8340084: Open source AWT Frame related tests
a07052e: 8340573: Remove unused G1ParScanThreadState::_partial_objarray_chunk_size
0f253d1: 8340392: Handle OopStorage in location decoder
f31f97d: 8340171: CDS: Enhance bitmap truncation
34cddfb: 8340387: Update OS detection code to recognize Windows Server 2025
dd49879: 8340524: Remove NarrowPtrStruct
ab06a87: 8340544: Optimize setLocalsFromArg
2461263: 8339217: Optimize ClassFile API loadConstant
... and 90 more: https://git.openjdk.org/jdk/compare/e1ebeef0405ac6e48564a035767ee256291b9ca9...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 · 2024-09-17T11:04:28Z

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

hotspot

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 · 2024-09-17T11:07:14Z

Webrevs

TobiHartmann · 2024-09-17T11:10:58Z

src/hotspot/share/opto/compile.cpp

@@ -719,6 +719,11 @@ Compile::Compile( ciEnv* ci_env, ciMethod* target, int osr_bci,
    method()->ensure_method_data();
  }

+  if (StressLCM || StressGCM || StressIGVN || StressCCP ||


We need to initialize the seed earlier because StressUnstableIfTraps is already used during parsing.

vnkozlov

Few comments.

src/hotspot/share/opto/parse2.cpp

TobiHartmann

Thanks for your review, Vladimir! I uploaded a new version and addressed your comments.

src/hotspot/share/opto/parse2.cpp

vnkozlov

More comments.

src/hotspot/share/opto/parse2.cpp

TobiHartmann

Thanks again Vladimir. I updated the code and answered your comments.

src/hotspot/share/opto/parse2.cpp

vnkozlov

Looks good now.

vnkozlov · 2024-09-19T16:51:36Z

What do you think?

Thank you for looking on this. I am fine with current code.

TobiHartmann · 2024-09-23T05:45:47Z

Thanks Vladimir!

chhagedorn

Small suggestion, otherwise, it looks good to me, too!

chhagedorn · 2024-09-23T06:06:13Z

src/hotspot/share/opto/parse2.cpp

+  }
+}
+
+void Parse::stress_trap(IfNode* orig_iff, Node* counter, Node* incr_store) {


Can you add a brief summary as method comment (i.e. following the explanation in the PR)? This would help to understand the reason why we want to stress traps, how it looks in the IR, and how frequently the trap is taken.

TobiHartmann · 2024-09-23T10:37:26Z

Thanks for the review Christian! I added a comment to Parse::stress_trap.

chhagedorn

Thanks for adding the comment! Great to have this new stress mode :-)

TobiHartmann · 2024-09-23T12:29:31Z

Thanks Christian!

TobiHartmann · 2024-09-23T12:29:40Z

/integrate

openjdk · 2024-09-23T12:30:32Z

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

37ec80d: 8339161: ZGC: Remove unused remembered sets
384deda: 8325949: Create an internal utility method for creating VarHandle instances
67448b0: 8339852: Fix typos in java.compiler documentation
bc7c0dc: 8340084: Open source AWT Frame related tests
a07052e: 8340573: Remove unused G1ParScanThreadState::_partial_objarray_chunk_size
0f253d1: 8340392: Handle OopStorage in location decoder
f31f97d: 8340171: CDS: Enhance bitmap truncation
34cddfb: 8340387: Update OS detection code to recognize Windows Server 2025
dd49879: 8340524: Remove NarrowPtrStruct
ab06a87: 8340544: Optimize setLocalsFromArg
... and 91 more: https://git.openjdk.org/jdk/compare/e1ebeef0405ac6e48564a035767ee256291b9ca9...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-23T12:30:40Z

@TobiHartmann Pushed as commit 63e611c.

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

TobiHartmann added 19 commits July 1, 2024 13:14

8335334: Stress mode to randomly execute unstable if traps

9af15d0

Fixes

7baafcf

Fix

c0a3d99

New version with Opaque node - still buggy

0cd3f66

New version with Opaque node - still buggy - more changes

66ed4af

v3

04c8b19

some fixes

80b437b

v4

081036a

Some more fixes

2ea473e

Merge branch 'master' into JDK-8335334

f653c0f

More randomization

50b22d0

Small fix

6ee626b

Test fix

ef0f011

Test fix

13b8cc7

Merge branch 'master' into JDK-8335334

ac51fd1

Small fix

9aa38e9

Randomly skip emitting an uncommon trap

3b14dc1

Test fix

0b613fe

Added stress test

8af8f42

openjdk bot added the rfr Pull request is ready for review label Sep 17, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Sep 17, 2024

TobiHartmann commented Sep 17, 2024

View reviewed changes

vnkozlov reviewed Sep 17, 2024

View reviewed changes

Review comments

3dbc584

TobiHartmann commented Sep 18, 2024

View reviewed changes

Moved declaration

8257e6e

vnkozlov reviewed Sep 18, 2024

View reviewed changes

src/hotspot/share/opto/parse2.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/parse2.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/parse2.cpp Show resolved Hide resolved

vnkozlov reviewed Sep 18, 2024

View reviewed changes

src/hotspot/share/opto/parse2.cpp Outdated Show resolved Hide resolved

More reviewer comments

691af16

TobiHartmann commented Sep 19, 2024

View reviewed changes

src/hotspot/share/opto/parse2.cpp Show resolved Hide resolved

src/hotspot/share/opto/parse2.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/parse2.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/parse2.cpp Outdated Show resolved Hide resolved

vnkozlov approved these changes Sep 19, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 19, 2024

chhagedorn approved these changes Sep 23, 2024

View reviewed changes

Added a comment

83830d8

openjdk bot removed the ready Pull request is ready to be integrated label Sep 23, 2024

chhagedorn approved these changes Sep 23, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 23, 2024

openjdk bot added the integrated Pull request has been integrated label Sep 23, 2024

openjdk bot closed this Sep 23, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8335334: Stress mode to randomly execute unstable if traps #21037

8335334: Stress mode to randomly execute unstable if traps #21037

TobiHartmann commented Sep 17, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 17, 2024

openjdk bot commented Sep 17, 2024 •

edited

Loading

openjdk bot commented Sep 17, 2024

mlbridge bot commented Sep 17, 2024 •

edited

Loading

TobiHartmann Sep 17, 2024

vnkozlov left a comment

TobiHartmann left a comment

vnkozlov left a comment

TobiHartmann left a comment

vnkozlov left a comment

vnkozlov commented Sep 19, 2024

TobiHartmann commented Sep 23, 2024

chhagedorn left a comment

chhagedorn Sep 23, 2024

TobiHartmann commented Sep 23, 2024

chhagedorn left a comment

TobiHartmann commented Sep 23, 2024

TobiHartmann commented Sep 23, 2024

openjdk bot commented Sep 23, 2024

openjdk bot commented Sep 23, 2024

8335334: Stress mode to randomly execute unstable if traps #21037

8335334: Stress mode to randomly execute unstable if traps #21037

Conversation

TobiHartmann commented Sep 17, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 17, 2024

openjdk bot commented Sep 17, 2024 • edited Loading

openjdk bot commented Sep 17, 2024

mlbridge bot commented Sep 17, 2024 • edited Loading

Webrevs

TobiHartmann Sep 17, 2024

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov commented Sep 19, 2024

TobiHartmann commented Sep 23, 2024

chhagedorn left a comment

Choose a reason for hiding this comment

chhagedorn Sep 23, 2024

Choose a reason for hiding this comment

TobiHartmann commented Sep 23, 2024

chhagedorn left a comment

Choose a reason for hiding this comment

TobiHartmann commented Sep 23, 2024

TobiHartmann commented Sep 23, 2024

openjdk bot commented Sep 23, 2024

openjdk bot commented Sep 23, 2024

TobiHartmann commented Sep 17, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 17, 2024 •

edited

Loading

mlbridge bot commented Sep 17, 2024 •

edited

Loading