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

8252219: C2: Randomize IGVN worklist for stress testing #242

Closed
wants to merge 14 commits into from

Conversation

@robcasloz
Copy link
Contributor

@robcasloz robcasloz commented Sep 18, 2020

Add StressIGVN option to let C2 randomize IGVN worklist order. When enabled, the worklist is shuffled before each main run of the IGVN loop. Also add GenerateStressSeed and StressSeed=N options to randomly generate or specify the seed. In either case, the seed is logged if LogCompilation is enabled. The new options are declared as production+diagnostic for consistency with the existing StressLCM and StressGCM options.


Progress

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

Issue

  • JDK-8252219: C2: Randomize IGVN worklist for stress testing

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/242/head:pull/242
$ git checkout pull/242

Add 'StressIGVN' option to let C2 randomize IGVN worklist order. When enabled,
the worklist is shuffled before each main run of the IGVN loop. Also add
'GenerateStressSeed' and 'StressSeed=N' options to randomly generate or specify
the seed. In either case, the seed is logged if 'LogCompilation' is enabled.

The generation or specification of seeds also affects the randomization
triggered by 'StressLCM' and 'StressGCM'. The new options are declared as
production+diagnostic for consistency with these existing options.
@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Sep 18, 2020

/summary
Add 'StressIGVN' option to let C2 randomize IGVN worklist order. When enabled,
the worklist is shuffled before each main run of the IGVN loop. Also add
'GenerateStressSeed' and 'StressSeed=N' options to randomly generate or specify
the seed. In either case, the seed is logged if 'LogCompilation' is enabled.

The generation or specification of seeds also affects the randomization
triggered by 'StressLCM' and 'StressGCM'. The new options are declared as
production+diagnostic for consistency with these existing options.

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 18, 2020

👋 Welcome back robcasloz! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 18, 2020

@robcasloz Setting summary to:

Add 'StressIGVN' option to let C2 randomize IGVN worklist order. When enabled,
the worklist is shuffled before each main run of the IGVN loop. Also add
'GenerateStressSeed' and 'StressSeed=N' options to randomly generate or specify
the seed. In either case, the seed is logged if 'LogCompilation' is enabled.

The generation or specification of seeds also affects the randomization
triggered by 'StressLCM' and 'StressGCM'. The new options are declared as
production+diagnostic for consistency with these existing options.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 18, 2020

@robcasloz 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 (add|remove) "label" command.

Loading

@robcasloz robcasloz marked this pull request as ready for review Sep 18, 2020
@openjdk openjdk bot added the rfr label Sep 18, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 18, 2020

Loading

@robcasloz robcasloz marked this pull request as draft Sep 20, 2020
@openjdk openjdk bot removed the rfr label Sep 20, 2020
@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Sep 20, 2020

Reverted to "draft mode", as I just realized the design is not repeatable since it relies on global PRNG state.

Loading

robcasloz added 4 commits Sep 21, 2020
Replace usage of global seed for stress testing with a seed per compilation, to
make stress runs repeatable in the face of concurrent method compilations. This
affects StressLCM, StressGCM, and StressIGVN. Reuse pseudonumber generation
logic in os::random_helper.
Use global seed for StressLCM and StressGCM as before to preserve their behavior
by now. In the future, these options can also benefit from using the local
(per-compilation) seed by simply using Compile::random() instead of os::random()
and extending the Compile constructor accordingly.
@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2020

@robcasloz Usage: /label <add|remove> [label[, label, ...]]wherelabel` is an additional classification that should be applied to this PR. These labels are valid:

  • serviceability
  • hotspot
  • sound
  • hotspot-compiler
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • 2d
  • security
  • swing
  • hotspot-runtime
  • jmx
  • build
  • nio
  • beans
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr
  • awt

Loading

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Sep 22, 2020

/label add hotspot-compiler

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2020

@robcasloz
The hotspot-compiler label was successfully added.

Loading

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Sep 22, 2020

/label remove hotspot

Loading

@openjdk openjdk bot removed the hotspot label Sep 22, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2020

@robcasloz
The hotspot label was successfully removed.

Loading

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Sep 22, 2020

/summary
Add 'StressIGVN' option to let C2 randomize IGVN worklist order. When enabled,
the worklist is shuffled before each main run of the IGVN loop. Also add
'GenerateStressSeed' and 'StressSeed=N' options to randomly generate or specify
the seed. In either case, the seed is logged if 'LogCompilation' is enabled.
The new options are declared as production+diagnostic for consistency with the
existing 'StressLCM' and 'StressGCM' options.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2020

@robcasloz Updating existing summary to:

Add 'StressIGVN' option to let C2 randomize IGVN worklist order. When enabled,
the worklist is shuffled before each main run of the IGVN loop. Also add
'GenerateStressSeed' and 'StressSeed=N' options to randomly generate or specify
the seed. In either case, the seed is logged if 'LogCompilation' is enabled.
The new options are declared as production+diagnostic for consistency with the
existing 'StressLCM' and 'StressGCM' options.

Loading

@robcasloz robcasloz marked this pull request as ready for review Sep 22, 2020
@openjdk openjdk bot added the rfr label Sep 22, 2020
@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Sep 22, 2020

This pull request is ready for review again.

Loading

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Sep 24, 2020

/summary
Add 'StressIGVN' option to let C2 randomize IGVN worklist order. When enabled,
the worklist is shuffled before each main run of the IGVN loop. Also add
'StressSeed=N' option to specify the seed. If the seed is not specified, a
random one is generated. In either case, the seed is logged if 'LogCompilation'
is enabled. The new options are declared as production+diagnostic for
consistency with the existing 'StressLCM' and 'StressGCM' options.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 24, 2020

@robcasloz Updating existing summary to:

Add 'StressIGVN' option to let C2 randomize IGVN worklist order. When enabled,
the worklist is shuffled before each main run of the IGVN loop. Also add
'StressSeed=N' option to specify the seed. If the seed is not specified, a
random one is generated. In either case, the seed is logged if 'LogCompilation'
is enabled. The new options are declared as production+diagnostic for
consistency with the existing 'StressLCM' and 'StressGCM' options.

Loading

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Thanks for making these changes. Looks good to me.

Loading

Copy link
Member

@chhagedorn chhagedorn left a comment

Looks good to me! Thanks for adding the additional test.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 25, 2020

@robcasloz 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 JDK-8252219
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Loading

@openjdk openjdk bot added merge-conflict and removed ready labels Sep 25, 2020
@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Sep 25, 2020

Nice!
Did you try to run mach5 testing with IGV stress enabled by using --jvm-args "" mach5 option?

Thanks Vladimir! I tried something similar (but hackier) on tier1 and all tests passed, will try a more systematic run with multiple seeds, etc.

I ran tier1_compiler with -XX:+StressIGVN for 100 repetitions and did not trigger any failure.

Loading

@openjdk openjdk bot added ready and removed merge-conflict labels Sep 25, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 25, 2020

Mailing list message from Roberto Castaneda Lozano on hotspot-compiler-dev:

"Merge master" (c2c31c3) addresses a trivial conflict caused by the
integration of JDK-8253586 (https://github.com//pull/334).

On 2020-09-25 09:54, Roberto Castaneda Lozano wrote:

Loading

@robcasloz
Copy link
Contributor Author

@robcasloz robcasloz commented Sep 28, 2020

/integrate

Loading

@openjdk openjdk bot added the sponsor label Sep 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 28, 2020

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

Loading

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Sep 28, 2020

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 28, 2020

@TobiHartmann @robcasloz Since your change was applied there have been 20 commits pushed to the master branch:

  • 625a935: 8253638: Cleanup os::reserve_memory and remove MAP_FIXED
  • f014854: 8253624: gtest fails when run from make with read-only source directory
  • 7817963: 8247691: [aarch64] Incorrect handling of VM exceptions in C1 deopt stub/traps
  • 79904c1: 8252730: jlink does not create reproducible builds on different servers
  • ea7c47c: 7179006: [macosx] Print-to-file doesn't work: printing to the default printer instead.
  • b66fa8f: 8253572: [windows] CDS archive may fail to open with long file names
  • 4167540: 8253647: Remove dead code in os::create_thread() on Linux/BSD
  • 5a57945: 8231591: [TESTBUG] Create additional two phase jpackage tests
  • b159e4e: 8253149: Building an installer from invalid app image fails on Window…
  • 0e855fe: 8252377: Incorrect encoding for EC AlgorithmIdentifier
  • ... and 10 more: https://git.openjdk.java.net/jdk/compare/37b70282b5bba1033eaa06fe64ac5438168c1be3...master

Your commit was automatically rebased without conflicts.

Pushed as commit fed3636.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants