RFR: 8361055: Serial: Inline SerialHeap::process_roots [v4]

[Thomas Schatzl]

On Tue, 15 Jul 2025 08:29:23 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> 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
>
> Albert Mingkun Yang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into sgc-roots
>  - review
>  - Merge branch 'master' into sgc-roots
>  - review
>  - sgc-roots

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:
https://github.com/openjdk/jdk/commit/e4355dc689afa56f214588630a15d911737422a9 (cleanup)
https://github.com/openjdk/jdk/commit/9a0a4ff27d8ae6fdea59592db3571e9e5cdbccd7 (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.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/26038#issuecomment-3089331707