8034761: Remove the do_code_roots parameter from process_strong_roots

[Stefan Karlsson]

On 13/02/14 09:56, Mikael Gerdin wrote:
> On Thursday 13 February 2014 09.49.01 Stefan Karlsson wrote:
>> Mikael,
>>
>> On 13/02/14 09:30, Mikael Gerdin wrote:
>>> Stefan,
>>>
>>> On Wednesday 12 February 2014 10.43.33 Stefan Karlsson wrote:
>>>> Hi all,
>>>>
>>>> Please, review this patch to remove the do_code_roots parameter from
>>>> SharedHeap::process_strong_roots.
>>>>
>>>> The changes done are:
>>>> - Change the code to rely on the ScannningOption so parameter instead of
>>>> do_code_roots.
>>>> - Change GenMarkSweep and G1MarkSweep to adjust the code roots with the
>>>> help of process_strong_roots instead of doing it as a separate phase
>>>> after process_strong_roots.
>>>> - Removed the unused FalseClosure.
>>>>
>>>> After this patch the adjust phase of the GenMarkSweep and G1MarkSweep
>>>> will use the generic code in process_strong_roots, which mark/claim the
>>>> nmethods before they are processed. Before the patch these two Serial
>>>> Old GC adjust phases skipped the mark/claim part. No noticeable Serial
>>>> Old GC time increases were found when this patch was performance tested.
>>>>
>>>> This cleanup is needed/wanted for G1 Class Unloading.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~stefank/8034761/webrev.00/
>>> I think the change is good but I have a comment about the changes to G1's
>>> verification code:
>>>
>>> 3399     // We apply the relevant closures to all the oops in the
>>> 3400     // system dictionary, the string table and the code cache.
>>> 3401     const int so = SO_AllClasses | SO_Strings | SO_AllCodeCache;
>>> 3402
>>>
>>> Shouldn't you remove SO_AllCodeCache here since you plan to go through the
>>> entire code cache just below?
>>> Otherwise you're just going to do all the nmethod liveness verification
>>> work twice since G1VerifyCodeRootOopClosure only adds verification
>>> (compared to VerifyRootsClosure) if the non-default flag
>>> +G1VerifyHeapRegionCodeRoots is set.
>> Yes, you're right.
>>
>> http://cr.openjdk.java.net/~stefank/8034761/webrev.01.delta
>> http://cr.openjdk.java.net/~stefank/8034761/webrev.01
> Looks good, ship it!

Thanks!.

StefanK

>
> /m
>
>> thanks for the review,
>> StefanK
>>
>>> /Mikael
>>>
>>>> RFE:
>>>> https://bugs.openjdk.java.net/browse/JDK-8034761
>>>>
>>>> thanks,
>>>> StefanK