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

[Erik Österlund]

Hi,

Thanks for the review Roman!

/Erik

On 2017-07-25 15:28, Roman Kennke wrote:
> 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