RFR (M): 8159984: Remove call to ClassLoaderDataGraph::clear_claimed_marks during the initial mark pause

[Stefan Johansson]

Hi Thomas,

On 2019-09-20 10:02, Thomas Schatzl wrote:
> Hi,
> 
> On 19.09.19 19:27, Kim Barrett wrote:
>>> On Sep 19, 2019, at 5:42 AM, Thomas Schatzl 
>>> <thomas.schatzl at oracle.com> wrote:
>>>
>>> Hi Kim,
>>>
>>>   thanks for your review.
>>>
>>> On 18.09.19 20:36, Kim Barrett wrote:
>>>>> On Sep 18, 2019, at 11:22 AM, Thomas Schatzl 
>>>>> <thomas.schatzl at oracle.com> wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>>   can I have reviews for this change that, given the observation 
>>>>> that during root processing, there is only one kind of root that 
>>>>> walks CLDs, we do not need to
>>>>>
>>>>> [...]
>>>>>
>>>>> (based on JDK-8231117)
>>>>>
>>>>> Thanks,
>>>>>   Thomas
>>>> I was just looking at the root scanning code recently, trying to
>>>> figure out what worker_has_discovered_all_strong_classes() was about.
>>>> The naming of worker_has_discovered_all_strong_classes() and
>>>> wait_until_all_strong_classes_discovered() seems wrong now.  These are
>>>> about discovering/processing strong nmethods now.
>>>> A better comment at the call to the wait function about why it's
>>>> called there would be helpful.  Obviously we want it as late as
>>>> possible, to allow threads to get past the notification.  But why not
>>>> later?
>>>
>>> We could at the latest do the waiting when we do the nmethod weak 
>>> root scanning. Given that I think I now have a working prototype that 
>>> works without the barrier I did not care optimizing that for this 
>>> change.
>>> If there are any unforeseen issues, or it does not pan out, I can 
>>> improve that (in a separate CR if needed).
>>>
>>> I fixed the naming of the two methods and tried to improve the 
>>> comment in a new webrev.
>>>
>>> New webrevs:
>>> http://cr.openjdk.java.net/~tschatzl/8159984/webrev.0_to_1/ (diff)
>>> http://cr.openjdk.java.net/~tschatzl/8159984/webrev.1/ (full)
>>
>> The additional comments look good.  But shouldn’t the names of those 
>> functions refer to “nmethods"
>> rather than (generic) “roots”?  That is,
>>
>> worker_has_discovered_all_strong_nmethods
>> wait_until_all_strong_nmethods_discovered
>>
>> There are certainly strong roots that get looked at after the 
>> has_discovered call.
> 
>    fixed in place as suggested. The naming is indeed overloaded here; I 
> wanted to make the names more generic than they need to be.
> 
Looks good, just one minor comment that you can ignore if you don't 
agree and no need for a new webrev. I think the parameter 
notify_claimed_roots_done should also follow the "nmethod naming scheme".

Thanks,
Stefan

> Thanks,
>    Thomas