8361055: Serial: Inline SerialHeap::process_roots

[albertnetymk]

Refactor roots processing in Serial (young-gc and full-gc) to clean up the control-flow and make is clearer what roots and closures are used in each context.

Test: tier1-8

---

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-8361055: Serial: Inline SerialHeap::process_roots (Enhancement - P4)

Reviewers

• Thomas Schatzl (@tschatzl - Reviewer)
• Stefan Karlsson (@stefank - Reviewer) Review applies to 0743d5e5
• Kim Barrett (@kimbarrett - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26038

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8361055: Serial: Inline SerialHeap::process_roots

Reviewed-by: tschatzl, kbarrett, stefank

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

• e756c0d: 8361726: Shenandoah: More detailed evacuation instrumentation
• 89fe586: 8363837: Make StubRoutines::crc_table_adr() into platform-specific method
• 9e209fe: 8364110: Remove unused methods in GCCause
• ... and 76 more: https://git.openjdk.org/jdk/compare/7da274ded4a36c6314702b687fcafcda80ae08c4...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]

@albertnetymk The following label will be automatically applied to this pull request:

• hotspot-gc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 04: Full - Incremental (ab4088c0)
• 03: Full - Incremental (59432994)
• 02: Full - Incremental (8b779888)
• 01: Full - Incremental (0743d5e5)
• 00: Full (6088695a)

[tschatzl]

Not really convinced about this change: if there is need to add a root now, you need to look at three places instead of one.
The parameter to select particular additional roots seems clear enough to me.

[stefank]

Suggested change

	_old_gen->scan_old_to_young_refs(_old_gen->space()->top());
	// During young-gc, visit all (strong+weak) clds with the same closure.
	_old_gen->scan_old_to_young_refs(_old_gen->space()->top());
	// During young-gc, visit all (strong+weak) clds with the same closure.

I also wonder if the previous placement of scan_old_to_young_refs after the other roots were intentional and this PR changes that?

[albertnetymk]

Both orderings are fine. (ScavengeRootsTask processes old-to-young pointers first). I changed that to avoid a local variable. Can revert it back if you prefers that.

[stefank]

OK. I understand. It could be worth adding a comment explaining that we're scanning objects that were old before relocation started. FWIW, if you want to keep it at the top then I think you can get rid of "_old_gen->space()->top()" from this function and extract it inside scan_old_to_young_refs instead.

[albertnetymk]

Added comment and inlined space()->top() to the callee.

[stefank]

The old code treated !ClassUnloading differently. It's not clear to me why this change is correct.

[albertnetymk]

always_strong_cld_do does ClassUnloading checking internally.

Semantically, for full-gc marking, we process always-strong clds only. (Ofc, if class unloading is off, all clds are always-strong.)

[stefank]

I see.

After having not looked at this for a while I find the "always" part of the name both redundant and even slightly misleading. This function could be named "strong_cld_do" (or even strong_clds_do, but the other functions are also miss the s). This could be a cleanup for the future.

[albertnetymk]

Not really convinced about this change: if there is need to add a root now, you need to look at three places instead of one.

That's true and IMO desirable. If a new kind of roots are added, we need to think carefully, how (young/old) gcs and different phases process it.

The parameter to select particular additional roots seems clear enough to me.

Well, I guess it's different style. For example, with this refactoring (untanglement), I noticed that for young-gc, Threads::oops_do doesn't need the second argument (nmethod_cl can be set to null) to skip nmethod scanning. I suspect this redundancy will be hard to spot in the original style. (I will do that after this refactoring.)

[stefank]

Not really convinced about this change: if there is need to add a root now, you need to look at three places instead of one.

That's true and IMO desirable. If a new kind of roots are added, we need to think carefully, how (young/old) gcs and different phases process it.

The parameter to select particular additional roots seems clear enough to me.

Well, I guess it's different style. For example, with this refactoring (untanglement), I noticed that for young-gc, Threads::oops_do doesn't need the second argument (nmethod_cl can be set to null) to skip nmethod scanning. I suspect this redundancy will be hard to spot in the original style. (I will do that after this refactoring.)

FWIW, I tend to like these kind of untanglements. We've gone back and forth w.r.t. the style in the root processors and making them more generic tends to make the code harder to understand, IMHO.

[stefank]

To me this is OK. I'll leave it up to you and other reviewers to determine if this change should be made or not.

[tschatzl]

I still do not see that much advantage of the new code over the other but I am not blocking this.

[tschatzl]

Suggested change

	NMethodToOopClosure nmthod_cl(&adjust_pointer_closure, NMethodToOopClosure::FixRelocations);
	CodeCache::nmethods_do(&nmthod_cl);
	NMethodToOopClosure nmethod_cl(&adjust_pointer_closure, NMethodToOopClosure::FixRelocations);
	CodeCache::nmethods_do(&nmethod_cl);

[tschatzl]

Suggested change

	// During young-gc, visit all (strong+weak) clds with the same closure.
	ClassLoaderDataGraph::cld_do(&cld_cl);
	Threads::oops_do(&oop_cl, &nmethod_cl);
	ScavengableNMethods::nmethods_do(&nmethod_cl);
	OopStorageSet::strong_oops_do(&oop_cl);
	// During young-gc, visit all (strong+weak) clds with the same closure.
	ClassLoaderDataGraph::cld_do(&cld_cl);
	Threads::oops_do(&oop_cl, &nmethod_cl);
	OopStorageSet::strong_oops_do(&oop_cl);
	ScavengableNMethods::nmethods_do(&nmethod_cl);

Keep same order of CLDG, Thread roots and OopStorage iteration to keep comparability high.

[albertnetymk]

It's kind of intentional that Threads::oops_do(&oop_cl, &nmethod_cl); and ScavengableNMethods::nmethods_do(&nmethod_cl); are next to each other. This way, it becomes more obvious that Threads::oops_do don't need to visit nmethod, i.e. nmethod_cl can be null there.

[tschatzl]

The most obvious thing to me would be explicitly spelling it out instead of assuming some reader's intuition, but I see your point.

[kimbarrett]

As is, I'm not entirely convinced this change is an improvement. We now have 3
separate places that are fairly similar (each calls some collection of
functions and maybe some additional functions). It's not at all obvious what
the reasoning for the various differences are.

Interleaving comments in each place mentioning the missing calls, and an
explanatory comment in each place about the overall task would be helpful. The
SO_xxx flags and how they were used at least provided some hints for the
latter, but now even those hints are gone.

[albertnetymk]

Interleaving comments in each place mentioning the missing calls, and an
explanatory comment in each place about the overall task would be helpful.

Added comments.

[kimbarrett]

Interleaving comments in each place mentioning the missing calls, and an
explanatory comment in each place about the overall task would be helpful.

Added comments.

Sorry, but I'm not finding the new comments especially informative. They seem
like "what" rather than "why" comments. The code seems relatively obvious
about "what". It's the rationale that isn't obvious. Not only about what is
there in each place, but also about what is not.

With the old code, having it all in one place (*) with using-context
controlling the details, that "why" information was implicit but wasn't too
hard to puzzle out. With the proposed scattering that seems much harder (to
me, at least).

Though in the old code the names of the scanning options (and even the name of
the type) aren't all that great. Especially now that option flags are never
combined. (I think there used to be more flags and more detailed control? I
haven't dug through git history to remind myself of how we got here.) If
instead of the scan options we had something like an enum for "young
collection" and "full collection marking" and "full collection pointer
adjustment", I think that would be clearer than the existing code.

To be clear, I'm not dead set against the approach being proposed by this PR.
But such a change needs to be an improvement, and I don't think the current
proposal meets that requirement. Not even compared to the existing code, let
alone compared to tidying things up along the lines described in the previous
paragraph.

(*) The WeakProcessor::oops_do in fullgc pointer adjustment phase is perhaps
an outlier? Maybe it should have been in process_roots, with a relevant
option? That would have retained the option combining, or perhaps triggered a
move away from that style. Though this also harkens back to a long ago
discussion / dispute about the meaning of "root".

[albertnetymk]

Not only about what is there in each place, but also about what is not.

There are only three kinds of "real" roots -- CLDs, threads, and OopStorage -- and all need to be processed somehow for correctness. In various calling contexts (like young-GC, full-GC marking, or full-GC adjustment), we layer on some additional "pseudo" roots and apply specific closures tailored for those cases. The added comments calls out the additional "pseudo" roots explicitly in every calling context.

I believe this way of structuring the code -- where the handling of roots and pseudo-roots is explicit at each callsite -- results in a simpler and more comprehensible mental model. It avoids the complexity of having a generic method enumerate all possible roots and control the subset for each context via flags or options. Instead, readers can see directly what roots are processed right where it matters.

But such a change needs to be an improvement...

FWIW, this reveals a redundancy in young-gc root processing: #26038 (comment)

[tschatzl]

I played around with the two options, either cleaning up the existing code and keep the process_roots, and a self-written variant of this:
e4355dc (cleanup)
9a0a4ff (same suggestion as here)

(untested :) )

So I think while I still somewhat prefer the old style (everything in one place), there is reason to treat these differently, and so the second variant does make some sense.
Particularly with the optimization/changes this change introduced, there really is little common code.

What the second variant still does not help with is easy comparison of the three variants; I only found that probably when not class unloading, it is probably not necessary to go through the thread's nmethods (https://bugs.openjdk.org/browse/JDK-8362588) in the full gc mark phase. All(?) embedded oops should be reachable by InstanceKlass::_methods of all CLDs (or, if that is not sufficient, why isn't it required to, similar to G1/the adjust pointer phase to walk through the entire codecache)....

Maybe the just added comments could be improved more, see the examples in the above change to spell out exactly why particular parameters are chosen.

I particularly do not like this one:

// Only nmethods that contain pointers into-young need to be processed
// during young-gc, and they are tracked in ScavengableNMethods
Threads::oops_do(&oop_cl, &nmethod_cl);
ScavengableNMethods::nmethods_do(&nmethod_cl);

So the comment begs the question why the nmethod_cl is passed to Threads::oops_do if "they are tracked by ScavengableNMethod" - as has already been pointed out, this is unnecessary, but it is a bit awkward to me that the documentation in this change points out an issue :)

Also in the current change the code uses both _cl and _closure postfixes for the closures. Would be nice to use one.

Still undecided whether this change is an improvement, but not too concerned about it any more. If there is a set of additional roots common to all of them in the future, that one and the OopStorageSet one can be factored out.

[albertnetymk]

Updated comments. I kept the numbering as I find it helpful to capture what roots are dealt with in each context.

I particularly do not like this one:...

Revised. I was planning to fix that in a followup PR, as it's beyond "inlining".

[kimbarrett]

I'm still not entirely convinced that spreading things out is an improvement.
I think I would have preferred improvements to the centralized approach.
But this is better than what we currently have.

[albertnetymk]

Thanks for review.

/integrate

[openjdk]

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

• 3b0da29: 8364114: Test TestHugePageDecisionsAtVMStartup.java#LP_enabled fails when no free hugepage
• e307b5c: 8363584: Sort share/utilities includes
• 4189fcb: 8362596: RISC-V: Improve _vectorizedHashCode intrinsic
• ... and 83 more: https://git.openjdk.org/jdk/compare/7da274ded4a36c6314702b687fcafcda80ae08c4...master

Your commit was automatically rebased without conflicts.

[openjdk]

@albertnetymk Pushed as commit af5932e.

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