8370039: GenShen: array copy SATB barrier improvements

[earthling-amzn]

When an array copy happens concurrently with old and young marking, Shenandoah's generational mode walks over the array twice. This is unnecessary and increases the workload for marking threads. It also has been unconditionally enqueuing old references during a young mark. This is also unnecessary and also increases marking workload. Finally, the barrier went through a somewhat complicated decision process based on affiliation of the region where the array resides. However, the barrier must consider the affiliation of objects that are pointed at by array elements.

---

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-8370039: GenShen: array copy SATB barrier improvements (Enhancement - P4)

Reviewers

• Kelvin Nilsen (@kdnilsen - Committer)
• Y. Srinivas Ramakrishna (@ysramakrishna - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28183

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

Using diff file

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

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

8370039: GenShen: array copy SATB barrier improvements

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

• f971ee5: 8349192: jvmti/scenarios/contention/TC05/tc05t001 fails: ERROR: tc05t001.cpp, 281: (waitedThreadCpuTime - waitThreadCpuTime) < (EXPECTED_ACCURACY * 1000000)
• 7c169c9: 8365426: [macos26] Graphics2D tests fail on new macOS 26
• bc928c8: 5038439: Warning message for literal shift amounts outside the canonical domain
• ... and 114 more: https://git.openjdk.org/jdk/compare/cad73d39762974776dd6fda5efe4e2a271d69f14...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]

@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

• 02: Full - Incremental (ba71462d)
• 01: Full - Incremental (ddd3d6d9)
• 00: Full (3086fd30)

[kdnilsen]

Is the comment at the start of arraycopy_work() still relevant? The description of the PR suggests that we will no longer call arraycopy_work() twice, but I'm not sure I fully understand the context of that statement.

[earthling-amzn]

We used to call this twice (as arraycopy_marking for each object if young and old marking were both in progress). The comment at the start is trying to explain how the arraycopy_work could be called with different template parameters depending on the gc_state. It is confusingly worded, but I believe the comment is correct and the code is wrong here. The way the code is in the PR, it would effectively turn off the old SATB during young evacuation and young update-refs, which is not what we want. Good catch!

[ysramakrishna]

Thank you for the clean up and corrections.

I am curious how you found this issue -- nice catch!

Any performance data to share, may be even from a microbenchmark perhaps or any other benchmark that exercises array copying -- may be something in DaCapo/Renaissance?