RFR (S): 8185141: Generalize scavengeable nmethod root handling

[Erik Österlund]

Hi Roman,

I see your point. From my perspective, the default for any GC is to use 
the shared CodeCache scavenge root list, and anything else 
(G1/Shenandoah) is an exception and can override to do something else 
instead.

Having said that, I agree we could easily move that default 
implementation to CodeCache from CollectedHeap and call it explicitly 
where it is used so that we do not accidentally mess up when we build a 
new GC.

However, then I think we should also move verify_nmethod_roots() into 
those GCs then, as it is closely related to which list it is on.

New full webrev:
http://cr.openjdk.java.net/~eosterlund/8185141/webrev.01/

New incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8185141/webrev.00_01/

What do you think?

Thanks,
/Erik

On 2017-07-25 13:36, Roman Kennke wrote:
> Hi Erik,
>
> the change looks mostly good to me. This really needed cleanup.
>
> However, I question to do the default impl in CollectedHeap, and rely on
> G1 to override it. Shenandoah's not using the scavenge roots list
> either. It seems odd to have a default impl in the superclass that is
> used by only 2 subclasses (GCH and PSH), and 2 other subclasses not
> using it. And potential future implementors require to override it to
> not do that stuff. Think Epsilon GC too: it doesn't need it, and must
> add code to not do it. It just seems wrong. I'd just add the impl to
> both GCH and PSH, and leave the superclass empty.
>
> Roman
>
> Am 25.07.2017 um 13:29 schrieb Erik Österlund:
>> Hi,
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8185141
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8185141/webrev.00/
>>
>> There seems to be different ways of handling scavengeable nmethod
>> roots in hotspot.
>>
>> The primary way of dealing with them is to use the CodeCache scavenge
>> root nmethod list that maintains a list of all nmethods with
>> scavengeable nmethods.
>> However, G1 does not use this list as it has its own mechanism of
>> keeping track of nmethods with scavengeable roots pointing into the heap.
>> To handle this, the current CodeCache code is full of special cases
>> for G1. In multiple cases we check if (UseG1GC) and then return.
>> ,m
>> We seemingly need a better way of communicating to the GC what
>> scavengeable nmethod roots there are to be able to get rid of the if
>> (UseG1GC)... code.
>>
>> As a solution, I propose to make CollectedHeap::register_nmethod the
>> primary way of registering to the GC that there might be a new nmethod
>> to keep track of. It is then up to the specific GC to take appropriate
>> action. The default appropriate action of CollectedHeap is to add the
>> nmethod to the shared scavenge root nmethod list if it is not already
>> on the list and it detected the existence of a scavengeable root oop
>> in the nmethod. G1 on the other hand, will use its closures to figure
>> out what remembered set it should be added to.
>>
>> When using G1, the CodeCache scavenge list will be empty, and so a lot
>> of G1-centric code for exiting before we walk the list of nmethods on
>> the list can be removed where the list is processed in a for loop.
>> Because since the list is empty, it does not matter that G1 runs this
>> code too - it will just iterate 0 times in the loop since it is empty.
>> But that's because the list was empty, not because we are using G1 -
>> it just happens to be that the list is always empty when we use G1.
>>
>> Testing: JPRT with hotspot testset, RBT hs-tier3.
>>
>> Thanks,
>> /Erik
>