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

[Roman Kennke]

Much better! Good to go for me.

Roman

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