RFR: 8212663: Remove conservative at_safepoint assert when JFR writes type sets during class unloading

[Erik Österlund]

Hi David,

On 2018-10-19 09:52, David Holmes wrote:
> Hi Erik,
>
> On 19/10/2018 5:25 PM, Erik Österlund wrote:
>> Hi David,
>>
>> Isn't the single thread call context the assumed default?
>
> Not sure what you mean.
>
>> In particular, this is called by SystemDictionary::do_unloading() 
>> only. And none of that stuff would work if called by multiple threads.
>> So while it is indeed possible to distinguish the ZDriver thread as a 
>> special type of concurrent GC thread that also handles unloading, and 
>> verify there can only be one such thread with that role, for this 
>> assert, I'm not sure if it is worth the hassle as all code in the 
>> unloading path is single threaded anyway, and I can't see what is 
>> special about this code apart from that there was an assert there 
>> before and the other single threaded code did not. Do you agree?
>
> The author of the current code considered it desriable to check the 
> code was only called at a safepoint. That might mean it only executes 
> in the VMThread or it might not - the single-threaded aspect of this 
> code is not evident to me just from the code. If checking for a 
> safepoint was really a proxy for checking it was executed by the 
> VMThread, and concurrent unloading now uses a different thread, then 
> it didn't seem unreasonable to ask if the assertion could be modified 
> to cover that rather than just removed.

Don't get me wrong; I don't think the question is unreasonable. I do see 
your point, and the question is reasonable.
I suppose what I'm saying is that the whole call tree under 
SystemDictionary::do_unloading() is built for being called by a single 
thread (and will continue being so in the concurrent unloading world), 
including this JFR code, but I don't know how much it would help 
protecting us from messing up by sprinkling asserts throughout that call 
tree making sure we are indeed not calling do_unloading in parallel 
accidentally. Having said that, maybe an extra seat belt won't hurt either.

> If it's really too much trouble than that's fine, but it was worth 
> asking the question.

How about using assert_locked_or_safepoint(ClassLoaderDataGraph_lock); 
instead? The unloading thread will hold that lock throughout the 
unloading path.

Like this:
http://cr.openjdk.java.net/~eosterlund/8212663/webrev.01/

Thanks,
/Erik

> Cheers,
> David
>
>
>> Thanks,
>> /Erik
>>
>> On 2018-10-19 03:51, David Holmes wrote:
>>> Hi Erik,
>>>
>>> On 19/10/2018 1:00 AM, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> JFR writes type sets during class unloading. It currently asserts 
>>>> this is done in a safepoint. But in fact, it suffices that it is 
>>>> done by a single thread. This assert needs to be relaxed for 
>>>> concurrent class unloading.
>>>
>>> Okay but you removed it completely. Is there not something you can 
>>> assert to verify your single thread requirement:
>>>
>>> Thread::current()->is_VM_thread() || Thread::current()->is_ 
>>> classUnloader_thread()
>>>
>>> ??
>>>
>>> Cheers,
>>> David
>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8212663/webrev.00/
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8212663
>>>>
>>>> Thanks,
>>>> /Erik
>>