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

8325671: Shenandoah: Introduce a ShenandoahGenerationType and templatize certain marking closures with it #17815

Closed
wants to merge 10 commits into from

Conversation

ysramakrishna
Copy link
Member

@ysramakrishna ysramakrishna commented Feb 12, 2024

/issue JDK-8325671

/summary

In support of eventually supporting a generational version of Shenandoah, we introduce a ShenandoahGenerationType enum which currently has a single NON_GEN value for Shenandoah. The generational extension of Shenandoah will introduce additional enum values when GenShen is integrated. These will be used to specialize certain marking closures via templatization, such that suitable variations of the marking (and in the future other) closures can be used for the generational and non-generational variants of the collector while minimizing interference between the two variants while maximizing code-sharing without affecting correctness or performance.

This ticket introduces the new enum type and templatizes certain existing marking closures.

In effect, this should be semantically a no-op for current (non-generational) Shenandoah and ideally completely performance-neutral (to be established via measurements).

Testing:

  • jtreg hotspot_gc
  • github actions
  • codepipeline perf & stress

Performance:

  • specjbb w/shenandoah
  • codepipeline perf

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-8325671: Shenandoah: Introduce a ShenandoahGenerationType and templatize certain marking closures with it (Sub-task - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17815

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

Using diff file

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

Webrev

Link to Webrev Comment

The template expands for only the NON_GEN type for the non-generational
version of Shenandoah currently, and will in the future accomodate
Generational Shenandoah.
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 12, 2024

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

@openjdk
Copy link

openjdk bot commented Feb 12, 2024

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

@openjdk openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Feb 12, 2024
@ysramakrishna ysramakrishna marked this pull request as ready for review February 12, 2024 21:59
@openjdk openjdk bot changed the title [Draft] JDK-8325671: Shenandoah: Introduce a ShenandoahGenerationType and templatize certain marking closures with it 8325671: Shenandoah: Introduce a ShenandoahGenerationType and templatize certain marking closures with it Feb 12, 2024
@openjdk
Copy link

openjdk bot commented Feb 12, 2024

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

@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 12, 2024
@openjdk
Copy link

openjdk bot commented Feb 12, 2024

@ysramakrishna Setting summary to:

In support of eventually supporting a generational version of Shenandoah, we introduce a ShenandoahGenerationType enum which currently has a single NON_GEN value for Shenandoah. The generational extension of Shenandoah will introduce additional enum values when GenShen is integrated. These will be used to specialize certain marking closures via templatization, such that suitable variations of the marking (and in the future other) closures can be used for the generational and non-generational variants of the collector while minimizing interference between the two variants while maximizing code-sharing without affecting correctness or performance.

This ticket introduces the new enum type and templatizes certain existing marking closures.

In effect, this should be semantically a no-op for current (non-generational) Shenandoah and ideally completely performance-neutral (to be established via measurements).

**Testing:**
- [x] jtreg hotspot_gc
- [x] github actions
- [ ] codepipeline perf & stress: in progress

**Performance:**
- [ ] specjbb w/shenandoah
- [ ] codepipeline perf: in progress

@mlbridge
Copy link

mlbridge bot commented Feb 12, 2024

Webrevs

@ysramakrishna
Copy link
Member Author

ysramakrishna commented Feb 13, 2024

Did some spot checks and overll sanity checks to compare the libjvm.so contents before and after the change, and don't see any great difference (although the libjvm.so size did increase a tad). Here's one example of the new tempaltized vs old non-templatized versions (second column is the size from nm):

  • Before templating:
0000000015044416 0000000000000947 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<InstanceMirrorKlass, narrowOop>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015037600 0000000000000930 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<InstanceMirrorKlass, oopDesc*>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015029888 0000000000000883 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<InstanceClassLoaderKlass, oopDesc*>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015033552 0000000000000882 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<InstanceClassLoaderKlass, narrowOop>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015029040 0000000000000836 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<InstanceKlass, oopDesc*>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015032704 0000000000000835 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<InstanceKlass, narrowOop>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015038544 0000000000000564 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<InstanceRefKlass, oopDesc*>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015045376 0000000000000564 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<InstanceRefKlass, narrowOop>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015037104 0000000000000489 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<InstanceStackChunkKlass, oopDesc*>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015043920 0000000000000489 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<InstanceStackChunkKlass, narrowOop>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015036912 0000000000000182 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<ObjArrayKlass, oopDesc*>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015043728 0000000000000182 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<ObjArrayKlass, narrowOop>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000022615392 0000000000000056 b OopOopIterateDispatch<ShenandoahMarkRefsClosure>::_table
0000000015028784 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::init<InstanceClassLoaderKlass>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015028832 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::init<InstanceStackChunkKlass>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015028736 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::init<InstanceMirrorKlass>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015028688 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::init<InstanceRefKlass>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015028928 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::init<TypeArrayKlass>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015028880 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::init<ObjArrayKlass>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015028640 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::init<InstanceKlass>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000022615368 0000000000000008 b guard variable for OopOopIterateDispatch<ShenandoahMarkRefsClosure>::_table
0000000015029024 0000000000000001 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<TypeArrayKlass, oopDesc*>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
0000000015029008 0000000000000001 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure>::Table::oop_oop_iterate<TypeArrayKlass, narrowOop>(ShenandoahMarkRefsClosure*, oopDesc*, Klass*)
  • After templating:
0000000015038032 0000000000000947 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<InstanceMirrorKlass, narrowOop>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015044480 0000000000000930 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<InstanceMirrorKlass, oopDesc*>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015029696 0000000000000883 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<InstanceClassLoaderKlass, oopDesc*>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015033360 0000000000000882 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<InstanceClassLoaderKlass, narrowOop>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015028848 0000000000000836 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<InstanceKlass, oopDesc*>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015032512 0000000000000835 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<InstanceKlass, narrowOop>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015058048 0000000000000564 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<InstanceRefKlass, oopDesc*>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015038992 0000000000000564 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<InstanceRefKlass, narrowOop>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015043984 0000000000000489 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<InstanceStackChunkKlass, oopDesc*>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015037536 0000000000000489 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<InstanceStackChunkKlass, narrowOop>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015043792 0000000000000182 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<ObjArrayKlass, oopDesc*>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015037344 0000000000000182 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<ObjArrayKlass, narrowOop>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000022615392 0000000000000056 b OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::_table
0000000015028592 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::init<InstanceClassLoaderKlass>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015028640 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::init<InstanceStackChunkKlass>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015028544 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::init<InstanceMirrorKlass>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015028496 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::init<InstanceRefKlass>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015028736 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::init<TypeArrayKlass>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015028688 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::init<ObjArrayKlass>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015028448 0000000000000037 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::init<InstanceKlass>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000022615368 0000000000000008 b guard variable for OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::_table
0000000015028832 0000000000000001 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<TypeArrayKlass, oopDesc*>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)
0000000015028816 0000000000000001 t void OopOopIterateDispatch<ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >::Table::oop_oop_iterate<TypeArrayKlass, narrowOop>(ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, oopDesc*, Klass*)

@ysramakrishna
Copy link
Member Author

Difference in (release) libjvm.so sizes (second column is size, a difference of ~2.4KB, less than 0.01%):

4095288896 26968936 generation_type.libjvm.so
2020291223 26966488 master.libjvm.so

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.

Thanks for these refinements. Look good to me.

@ysramakrishna
Copy link
Member Author

ysramakrishna commented Feb 14, 2024

Thanks @kdnilsen !

Can I please get a review from @rkennke , @shipilev , or other jdk Reviewer familiar with Shenandoah. Thanks!

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

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

The change looks okay, but I do not see a good reason to introduce this into Shenandoah before Generational mode arrives. It is just extra (mostly dead) code to maintain, and it effectively hides what changes Generational needs to do in shared Shenandoah code. Maybe we should consider this as part of Generational integration PR?

src/hotspot/share/gc/shenandoah/shenandoahMark.cpp Outdated Show resolved Hide resolved
@ysramakrishna
Copy link
Member Author

ysramakrishna commented Feb 15, 2024

The change looks okay, but I do not see a good reason to introduce this into Shenandoah before Generational mode arrives. It is just extra (mostly dead) code to maintain, and it effectively hides what changes Generational needs to do in shared Shenandoah code. Maybe we should consider this as part of Generational integration PR?

Indeed the idea was to introduce this earlier than any Generational-specific code as a decomposition and refactoring of the work needed for generational mode, as I describe in the summary above, such that its effect was independently shown to be minimal & harmless, and making subsequent integration of Generational mode much smaller, simpler, and easier to review on its own.

@openjdk
Copy link

openjdk bot commented Feb 21, 2024

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

8325671: Shenandoah: Introduce a ShenandoahGenerationType and templatize certain marking closures with it

In support of eventually supporting a generational version of Shenandoah, we introduce a ShenandoahGenerationType enum which currently has a single NON_GEN value for Shenandoah. The generational extension of Shenandoah will introduce additional enum values when GenShen is integrated. These will be used to specialize certain marking closures via templatization, such that suitable variations of the marking (and in the future other) closures can be used for the generational and non-generational variants of the collector while minimizing interference between the two variants while maximizing code-sharing without affecting correctness or performance.

This ticket introduces the new enum type and templatizes certain existing marking closures.

In effect, this should be semantically a no-op for current (non-generational) Shenandoah and ideally completely performance-neutral (to be established via measurements).

**Testing:**
- [x] jtreg hotspot_gc
- [x] github actions
- [ ] codepipeline perf & stress: in progress

**Performance:**
- [ ] specjbb w/shenandoah
- [ ] codepipeline perf: in progress

Reviewed-by: kdnilsen, shade

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 Feb 21, 2024
@rkennke
Copy link
Contributor

rkennke commented Feb 21, 2024

Maybe as a compromise we can review this PR now but only ship it with the rest of Generational Shenandoah, when it arrives? Shouldn't be hard to maintain the PR until then, we don't currently expect many changes in the same code. WDYT?

@ysramakrishna
Copy link
Member Author

Maybe as a compromise we can review this PR now but only ship it with the rest of Generational Shenandoah, when it arrives? Shouldn't be hard to maintain the PR until then, we don't currently expect many changes in the same code. WDYT?

It'd be nicer to be able to ship this so:

  1. it gets regular testing on its own
  2. it can be shared with subsequent changes in preparation of Generational, which you may have seen more of recently
  3. it has the advantage of digestible small incremental chunks of change (pun not intended) that can be easily and independently reviewed on their own without having to produce a series of dependent PRs and maintaining them on a private branch as subsequent work proceeds

Meanwhile, I'll discuss other potential alternatives with the team and think more about the approach I've taken in this regard.

@ysramakrishna
Copy link
Member Author

Followed up offline w/Roman, and will check these in later today after a final round of merge-&-test.

Thanks everyone for your reviews and input!

@ysramakrishna
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Mar 2, 2024

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

  • f68a4b9: 8327105: compiler.compilercontrol.share.scenario.Executor should listen on loopback address only
  • a9c17a2: 8327108: compiler.lib.ir_framework.shared.TestFrameworkSocket should listen on loopback address only

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Mar 2, 2024

@ysramakrishna Pushed as commit f62f2ad.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated shenandoah shenandoah-dev@openjdk.org
4 participants