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

8315247: GenShen: Condition calls to post-write barrier code generation by a flag #313

Closed
wants to merge 9 commits into from

Conversation

ysramakrishna
Copy link
Member

@ysramakrishna ysramakrishna commented Aug 28, 2023

Protect the card barrier code generation by a new global ShenandoahCardBarrier, which is disabled by default and enabled for GenShen (generational mode Shenandoah) only. If the user forces the barrier flag to a value inconsistent with mode, we exit with an appropriate error message.

The intention of the change is two-fold:

  1. Make the card-barrier code in support of GenShen more idiomatic along the lines of existing barrier-gen code in Shenandoah
  2. Reduce and simplify to the extent possible the impact of changes from GenShen on shared/legacy Shenandoah code, so as to make reviews/audits of shared/legacy code a bit easier

The changes have been made for x86, aarch, and ppc, and tested as follows:

  1. I have tested on x86 on a cloud host running jtreg1-4 and SPECjbb.
  2. A round of aarch64 and x86 pipeline tests have run (but ran into existing assertion that William is fixing).
  3. Many thanks to Martin Doerr (SAP) for putting this through a "quick spin" on PPC
  4. RISC-V & S390 don't currently support generational mode Shenandoah.

Progress

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

Issue

  • JDK-8315247: GenShen: Condition calls to post-write barrier code generation by a flag (Task - P3)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 313

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

Using diff file

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

Webrev

Link to Webrev Comment

test at caller of post-write-barrier for ShenandoahCardBarrier, and
assert in post-barrier/generation code that we are in generational mode.
@bridgekeeper
Copy link

bridgekeeper bot commented Aug 28, 2023

👋 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.

Copy link
Contributor

@earthling-amzn earthling-amzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit - can we have a consistent, more helpful message for the assertions?

  assert(ShenandoahHeap::heap()->mode()->is_generational(), "Not needed");

Maybe:

  assert(ShenandoahHeap::heap()->mode()->is_generational(), "Expected generational mode here");

Copy link
Contributor

@kdnilsen kdnilsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good. Have you sent it through the test pipeline?

If we have this ShenandoahCardBarrier flag, I'd be inclined to use it everywhere we currently have the query heap->mode()->is_generational(). It ought to be more efficient. Before you make that global change, maybe we should confirm that William (and other interested reviewers) like that approach.

@ysramakrishna
Copy link
Member Author

Minor nit - can we have a consistent, more helpful message for the assertions?

  assert(ShenandoahHeap::heap()->mode()->is_generational(), "Not needed");

Maybe:

  assert(ShenandoahHeap::heap()->mode()->is_generational(), "Expected generational mode here");

Following Kelvin's suggestion, I removed any reference to generational mode in the barrier/generayion code itself, and instead just assert that the ShenandoahCardBarrier flag should be set. I am going to look for a suitable place during initialization of non-generational mode to assert that it isn't set. We do check and bail at generational mode initialization if the flag isn't set.

I used the message Did you mean to enable ShenandoahCardBarrier? if it isn't enabled, but I could replace it with Expecting ShenandoahCardBarrier set here in case that reads better in the error message, or ShenandoahCardBarrier should be set.

configurations. Exit with an error message if the user tries to change
it.
@ysramakrishna ysramakrishna marked this pull request as ready for review August 29, 2023 18:04
@ysramakrishna
Copy link
Member Author

This looks pretty good. Have you sent it through the test pipeline?

If we have this ShenandoahCardBarrier flag, I'd be inclined to use it everywhere we currently have the query heap->mode()->is_generational(). It ought to be more efficient. Before you make that global change, maybe we should confirm that William (and other interested reviewers) like that approach.

Did a spin through test pipeline and will run another following the latest changes. All looks good from what I can tell from tests, including jtreg's & SPECjbb as well.

Opened official PR from draft and will link JBS ticket momentarily.

@ysramakrishna ysramakrishna changed the title Draft: GenShen: refactor card marking barrier code 8312457: GenShen: Condition calls to post-write barrier code generation by a flag Aug 29, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 29, 2023
@ysramakrishna ysramakrishna changed the title 8312457: GenShen: Condition calls to post-write barrier code generation by a flag 8315247: GenShen: Condition calls to post-write barrier code generation by a flag Aug 29, 2023
@ysramakrishna
Copy link
Member Author

/issue JDK-8315247

@openjdk
Copy link

openjdk bot commented Aug 29, 2023

@ysramakrishna This issue is referenced in the PR title - it will now be updated.

@mlbridge
Copy link

mlbridge bot commented Aug 29, 2023

Webrevs

@ysramakrishna
Copy link
Member Author

Made changes stemming from review feedback & discussion. Please take another look and let me know how this looks. Thanks!

@openjdk
Copy link

openjdk bot commented Aug 29, 2023

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

8315247: GenShen: Condition calls to post-write barrier code generation by a flag

Reviewed-by: wkemper, kdnilsen

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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 29, 2023
@ysramakrishna
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Aug 29, 2023

Going to push as commit ade7796.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 29, 2023
@openjdk openjdk bot closed this Aug 29, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 29, 2023
@openjdk
Copy link

openjdk bot commented Aug 29, 2023

@ysramakrishna Pushed as commit ade7796.

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

@ysramakrishna ysramakrishna deleted the card_barrier branch September 1, 2023 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants