RFR: 8267653: Remove Mutex::_safepoint_check_sometimes [v2]

[Coleen Phillimore]

On 5/25/21 10:00 PM, David Holmes wrote:
> On 26/05/2021 11:53 am, Coleen Phillimore wrote:
>> On Wed, 26 May 2021 00:38:00 GMT, David Holmes <dholmes at openjdk.org> 
>> wrote:
>>
>>>> Coleen Phillimore has updated the pull request incrementally with 
>>>> one additional commit since the last revision:
>>>>
>>>>    Add assert that the thread is terminated before taking Heap_lock
>>>
>>> src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.cpp 
>>> line 543:
>>>
>>>> 541:   } else {
>>>> 542:     // Heap_lock protects external pending list
>>>> 543:     MonitorLocker ml(Heap_lock);
>>>
>>> For performance you could leave the _no_safepoint_check flag.
>>
>> I cannot leave it because there's an assert to verify that we don't 
>> try to take this lock without a safepoint check, since it was 
>> declared with safepoint_check_always.
>
> But that assert only applies to JavaThreads and this is being taken by 
> a GC thread isn't it?

It's true that if this is a non-Java thread, it wouldn't fire the assert 
that checks that the states are consistent.  I had to go study the code 
a bit to see that, which I wouldn't want to do over again.  But it would 
be very inconsistent in the code since all other Heap_lock acquisitions 
are not made with _no_safepoint_check, we would have to look again to 
see why this is special and other GC threads take Heap_lock without 
no_safepoint_check.

And it wouldn't be a meaningful optimization.

thanks,
Coleen

>
>>> src/hotspot/share/runtime/thread.cpp line 3420:
>>>
>>>> 3418:     // we'll never emerge out of the safepoint before the VM 
>>>> exits.
>>>> 3419:
>>>> 3420:     MutexLocker ml(Heap_lock);
>>>
>>> Can you add an assert before this that the thread is_terminated() as 
>>> that is the key to the mutex acquisition not doing a safepoint check.
>>
>> Ok, I can add:
>> +    assert(thread->is_terminated(), "must be terminated before 
>> acquiring Heap_lock");
>> and rerun tests.
>
> Thanks,
> David
>
>> -------------
>>
>> PR: https://git.openjdk.java.net/jdk/pull/4184
>>