8321815: Shenandoah: gc state should be synchronized to java threads only once per safepoint

[earthling-amzn]

Presently, every change to the gc change is distributed to all java threads. The gc state is only changed on safepoints. For applications with a very large number of java threads this may increase time on the safepoint. It would be better to synchronize the gc state only once per safepoint.

---

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-8321815: Shenandoah: gc state should be synchronized to java threads only once per safepoint (Enhancement - P4)

Reviewers

• Kelvin Nilsen (@kdnilsen - no project role) ⚠️ Review applies to 91a5cee4
• Y. Srinivas Ramakrishna (@ysramakrishna - Reviewer) ⚠️ Review applies to 93d0f4e3

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17112

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back wkemper! 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]

@earthling-amzn The following labels will be automatically applied to this pull request:

• hotspot-gc
• shenandoah

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.

[mlbridge]

Webrevs

• 03: Full - Incremental (eb6f3fdf)
• 02: Full - Incremental (d1a4a692)
• 01: Full - Incremental (93d0f4e3)
• 00: Full (91a5cee4)

[kdnilsen]

Am wondering if we measure any improvement in time-at-safepoints for workloads with large number of threads?

[ysramakrishna]

This seems reasonable. How many unnecessary state change distributions to threads do we save from this, so as to quantify the savings?

It would seem as if, as a verification mechanism that no state changes were dropped from distribution, it might be a good idea to assert in both prologue and epilogue of ShenandoahHeap stop-world ops that the _gc_state_changed flag is false, indicating that the last state change was "consumed and distributed" by any stop-world op?

[earthling-amzn]

This instrumentation isn't included in this PR, but it shows how often GC state changes in a typical cycle:

[40.118s][31676][info ][gc,start    ] GC(1) Pause Init Mark (unload classes)
[40.118s][31676][info ][gc          ] GC(1) GC state = 22
[40.118s][31676][info ][gc          ] GC(1) Pause Init Mark (unload classes) 0.119ms
[41.646s][31676][info ][gc,start    ] GC(1) Pause Final Mark (unload classes)
[41.646s][31676][info ][gc          ] GC(1) GC state = 0
[41.646s][31676][info ][gc          ] GC(1) GC state = 10
[41.648s][31676][info ][gc          ] GC(1) GC state = 14
[41.648s][31676][info ][gc          ] GC(1) GC state = 15
[41.648s][31676][info ][gc          ] GC(1) Pause Final Mark (unload classes) 2.324ms
[42.774s][31676][info ][gc,start    ] GC(1) Pause Init Update Refs
[42.774s][31676][info ][gc          ] GC(1) GC state = 11
[42.774s][31676][info ][gc          ] GC(1) GC state = 1
[42.774s][31676][info ][gc          ] GC(1) GC state = 9
[42.774s][31676][info ][gc          ] GC(1) Pause Init Update Refs 0.108ms
[43.040s][31676][info ][gc,start    ] GC(1) Pause Final Update Refs
[43.040s][31676][info ][gc          ] GC(1) GC state = 1
[43.040s][31676][info ][gc          ] GC(1) GC state = 0
[43.040s][31676][info ][gc          ] GC(1) Pause Final Update Refs 0.162ms

And for some degenerated cycles, many of these changes may happen on the same safepoint:

[60.839s][31676][info ][gc,start       ] GC(17) Pause Degenerated GC (Mark)
[61.417s][31676][info ][gc             ] GC(17) GC state = 0
[62.291s][31676][info ][gc             ] GC(17) GC state = 4
[62.291s][31676][info ][gc             ] GC(17) GC state = 5
[62.309s][31676][info ][gc             ] GC(17) GC state = 1
[62.309s][31676][info ][gc             ] GC(17) GC state = 1
[62.309s][31676][info ][gc             ] GC(17) GC state = 9
[63.372s][31676][info ][gc             ] GC(17) GC state = 1
[63.372s][31676][info ][gc             ] GC(17) GC state = 0
[63.373s][31676][info ][gc             ] GC(17) Pause Degenerated GC (Mark) 31103M->24719M(32768M) 2534.356ms

I can add the assertions you described in the prologue and epilogue methods. For what it's worth, changes to the gc state already assert the changes occur on a safepoint.

[ysramakrishna]

Looks good; thanks for the data and for the extra verification checks.
🚢

[openjdk]

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

8321815: Shenandoah: gc state should be synchronized to java threads only once per safepoint

Reviewed-by: kdnilsen, ysr

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

• 459957f: 8322062: com/sun/jdi/JdwpAllowTest.java does not performs negative testing with prefix length
• b98d13f: 8259637: java.io.File.getCanonicalPath() returns different values for same path
• 4f3de09: 8321940: Improve CDSHeapVerifier in handling of interned strings
• 1fde8b8: 8321933: TestCDSVMCrash.java spawns two processes
• 66aeb89: 8315462: [REDO] runtime/NMT/SummarySanityCheck.java failed with "Total committed (MMMMMM) did not match the summarized committed (NNNNNN)"
• a5122d7: 8314029: Add file name parameter to Compiler.perfmap
• c0a3b76: 8316197: Make tracing of inline cache available in unified logging
• 7e1d26d: 8322287: Parallel: Remove unused arg in adjust_eden_for_pause_time and adjust_eden_for_minor_pause_time
• 5584ba3: 8322097: Serial: Refactor CardTableRS::find_first_clean_card
• 75d382d: 8322204: Parallel: Remove unused _collection_cost_margin_fraction
• ... and 42 more: https://git.openjdk.org/jdk/compare/1b621f5527a0d7ae345d79f293357446ab7876d9...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@ysramakrishna) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[earthling-amzn]

/integrate

[openjdk]

@earthling-amzn
Your change (at version eb6f3fd) is now ready to be sponsored by a Committer.

[ysramakrishna]

/sponsor

[openjdk]

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

• 459957f: 8322062: com/sun/jdi/JdwpAllowTest.java does not performs negative testing with prefix length
• b98d13f: 8259637: java.io.File.getCanonicalPath() returns different values for same path
• 4f3de09: 8321940: Improve CDSHeapVerifier in handling of interned strings
• 1fde8b8: 8321933: TestCDSVMCrash.java spawns two processes
• 66aeb89: 8315462: [REDO] runtime/NMT/SummarySanityCheck.java failed with "Total committed (MMMMMM) did not match the summarized committed (NNNNNN)"
• a5122d7: 8314029: Add file name parameter to Compiler.perfmap
• c0a3b76: 8316197: Make tracing of inline cache available in unified logging
• 7e1d26d: 8322287: Parallel: Remove unused arg in adjust_eden_for_pause_time and adjust_eden_for_minor_pause_time
• 5584ba3: 8322097: Serial: Refactor CardTableRS::find_first_clean_card
• 75d382d: 8322204: Parallel: Remove unused _collection_cost_margin_fraction
• ... and 42 more: https://git.openjdk.org/jdk/compare/1b621f5527a0d7ae345d79f293357446ab7876d9...master

Your commit was automatically rebased without conflicts.

[openjdk]

@ysramakrishna @earthling-amzn Pushed as commit 808a039.

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

[shipilev]

(Late for the party here!)

The idea looks good, but there are two minor problems with it:

1. Thread-local gc-state is supposed to be used from the barriers. This is why we can coalesce the thread-local gc-state within a safepoint, as long as we have the proper setup after the safepoint. However, I think it is a happy accident that we don't poll thread-local gc-state from current native barrier code, and thread-local gc-state is only used by java threads. If we ever did access from non-java (gc) threads, this would subtly break GC within e.g. final-mark vmop.
2. It is fairly weird that now ShenandoahHeap::set_gc_state_all_threads does the actual modification, while ShenandoahHeap::set_gc_state_mask does not! The names of the methods do not highlight the difference in semantics.

I guess we can hit two birds with one stone if we rename the methods to mention java threads and record/set split, and assert that non-java threads do not touch the ShenandoahThreadLocalData::_gc_state. Something like SH::set_gc_state (changes the global, sets the propagation flag) and SH::propagate_gc_state_java_threads (propagates global to thread-local java threads)?

[earthling-amzn]

I'll open a new PR to rename these methods and add an assert that non-java threads never touch the thread local copy of gc state.