RFR: 8327997: G1: Move G1ScanClosureBase::reference_iteration_mode to subclass [v2]

[Thomas Schatzl]

On Thu, 14 Mar 2024 10:02:53 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1OopClosures.hpp line 69:
>> 
>>> 67:     G1ScanClosureBase(g1h, pss), _heap_roots_found(heap_roots_found) { }
>>> 68: 
>>> 69:   // Because this closure is applied on pointers residing outside the
>> 
>> I think the comment is okay, but maybe should put more emphasis on that specifying this iteration mode allows _the compiler_ to (more easily) compile out the check in `try_discover`.
>> 
>> Also since this isn't the only place this (and similar) optimizations are made elsewhere too using this getter, I kind of agree with @lgxbslgx that the impact of the selection should probably be documented at the enum level (the move of the override into the leaf class here is fine with me).
>> 
>> Just something like "// Selection of one or the other option helps the compiler to remove unnecessary, known beforehand behavior for a given oop closure at compile time" seems sufficient to me there instead of this fairly long comment.
>> The exact changes can be easily gathered by searching the usages of the enum.
>
> I dropped the comment. This optimization, afaics, is only used/implemented in G1. Moving the comment to the enum level can be confusing, since its impact might not be visible (or even positive) for other collectors. (I inspected the generated asm of `g1Remset` with and without this optimization and they are indeed different -- the one *without* the optimization is shorter (in terms of LOC), but it's unclear which one is faster.)

I can see multiple references to the `DO_FIELDS` value in Hotspot code in that getter in several GCs, but only grepped for it without looking into detail about the reasons.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18244#discussion_r1524583117