shenandoah 8328235: GenShen: Robustify ShenandoahGCSession and fix missing use

/issue JDK-8328235

~ShenandoahGCSession is intended to create a scope where the ShenandoahHeap's _gc_cause and _gc_generation field reflect the current gc cycle. We now check that we do not overwrite existing non-default settings (respectively _no_gc and nullptr). The destructor of the scope/stack object also resets these fields to their default settings, ensuring intended uses. This uncovered a situation where the scope was not entered when it should have been, which we have now fixed. A case of flickering of active_generation() was identified, and found to be benign. An assert now checks for this situation. The code has been made robust wrt the flickering (seen only by mutators executing load barriers).~ To be fixed.

Testing:

[x] code pipeline
[x] specjbb
[x] jtreg:hotspot_gc and jtreg:hotspot:tier1 w/fastdebug
[x] GHA

Progress

[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8328235: GenShen: Robustify ShenandoahGCSession and fix missing use (Sub-task - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 407

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/shenandoah/pull/407.diff

Webrev

Link to Webrev Comment

Mar 15 '24 01:03 ysramakrishna

:wave: Welcome back ysr! 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.

Mar 15 '24 01:03 bridgekeeper[bot]

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

8328235: GenShen: Robustify ShenandoahGCSession and fix missing use

Reviewed-by: wkemper

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

Mar 15 '24 01:03 openjdk[bot]

@ysramakrishna The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

Mar 15 '24 01:03 openjdk[bot]

Webrevs

Mar 15 '24 01:03 mlbridge[bot]

Code pipeline testing revealed a further scenario when active_generation() was used outside of what should have been a gc session (temporal) scope. I'll revert this to a draft until I fix that issue as well.

Mar 15 '24 17:03 ysramakrishna

A case of flickering of active_generation() was identified, and found to be benign. An assert now checks for this situation. The code has been made robust wrt the flickering.

Mar 20 '24 10:03 ysramakrishna

A case of flickering of active_generation() was identified, and found to be benign. An assert now checks for this situation. The code has been made robust wrt the flickering.

Testing revealed an issue with this PR, in particular with the clearing of active_generation() via Scopes -- this needs closer examination, so I'll once again take this back to draft. Sorry for the, errr, flickering in this PR :-)

Mar 20 '24 19:03 ysramakrishna

@ysramakrishna 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 active_generation
git fetch https://git.openjdk.org/shenandoah.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

May 15 '24 20:05 openjdk[bot]

Publishing the draft for feedback.

Still needs a few cleanups. Not too happy with the minutiae of the mechanics that need to be better abstracted.

May 17 '24 16:05 ysramakrishna

Left a few minor coding suggestings. On a higher level, it's not clear to me when and which threads should use gc_generation or active_generation. I gather that active_generation is set on the safepoint and gc_generation is set by control thread, does that mean any code before init-mark needs to use gc_generation?

Thanks for the review. I'll make the suggested changes and respond individually to the feedback.

As regards, gc_generation vs active_generation, the intention is that only the load-reference-barrier and the mutator threads use active_generation and the GC worker threads use gc_generation (unless executing the load-reference-barrier). The idea is that the LRB's notion of an active generation cannot be updated safely except at a safepoint. I suspect this splitting of roles still presents a problem when evacuations occur during mixed collections. So there is likely more work to do to clean up the two roles.

More generally, gc_generation is tightly controlled by scopes by the coordinator thread, and indicate the generation that GC worker threads are dealing with at any time, except during evacuation/update-refs when the load-reference-barrier uses the active_generation.

I suspect there is still some badness or mix-ups in these LRB paths that is leading to issues that need to be resolved (there are some failures since a recent merge that I am yet to resolve).

Once I am done with those, I hope to have a cleaner solution.

Meanwhile, I am going to go over your suggestions and make the changes stemming from them.

May 28 '24 13:05 ysramakrishna

Thanks for your review, William!

/integrate

Jul 02 '24 01:07 ysramakrishna

Going to push as commit d2102347ea9c1199221ec33f4e721aefa1193cea.

Jul 02 '24 01:07 openjdk[bot]

@ysramakrishna Pushed as commit d2102347ea9c1199221ec33f4e721aefa1193cea.

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

Jul 02 '24 01:07 openjdk[bot]

shenandoah shenandoah copied to clipboard

8328235: GenShen: Robustify ShenandoahGCSession and fix missing use

Progress

Issue

Reviewing

Webrev

Webrevs

shenandoah
shenandoah copied to clipboard