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

[Erik Osterlund]

Hi Coleen,

Thanks for the review.

/Erik

> On 19 Oct 2018, at 15:13, coleen.phillimore at oracle.com wrote:
> 
> 
> The assert_locked_or_safepoint(ClassLoaderDataGraph_lock); looks good to me.  We've been doing that.
> Thanks,
> Coleen
> 
>> On 10/19/18 4:48 AM, Erik Österlund wrote:
>> 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
>>>> 
>> 
>