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

[David Holmes]

Hi Erik,

The locked_or_safepoint is good!

Thanks,
David

On 19/10/2018 6:48 PM, 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
>>>
>