RFR(s): 8234086: VM operation can be simplified

[David Holmes]

Hi Robbin,

Incremental v4 looks good.

Thanks,
David

On 23/11/2019 12:39 am, Robbin Ehn wrote:
> Hi David,
> 
> On 11/22/19 7:13 AM, David Holmes wrote:
>> Hi Robbin,
>>
>> On 21/11/2019 9:50 pm, Robbin Ehn wrote:
>>> Hi,
>>>
>>> Here is v3:
>>>
>>> Full:
>>> http://cr.openjdk.java.net/~rehn/8234086/v3/full/webrev/
>>
>> src/hotspot/share/runtime/synchronizer.cpp
>>
>> Looking at the highly discussed:
>>
>> if (Atomic::load(&ForceMonitorScavenge) == 0 && Atomic::xchg (1, 
>> &ForceMonitorScavenge) == 0) {
>>
>> why isn't that just:
>>
>> if (Atomic::cmpxchg(1, &ForceMonitorScavenge,0) == 0) {
>>
>> ??
> 
> I assumed someone had seen contention on ForceMonitorScavenge.
> Many threads can be enter and re-enter here.
> I don't know if that's still the case.
> 
> Since we only hit this path when the deprecated MonitorsBound is set, I 
> think I can change it?
> 
>>
>> Also while we are here can we clean this up further:
>>
>> static volatile int ForceMonitorScavenge = 0;
>>
>> becomes
>>
>> static int _forceMonitorScavenge = 0;
>>
>> so the variable doesn't look like it came from globals.hpp :)
>>
> 
> Sure!
> 
>> Just to be clear, I understand the changes around monitor scavenging 
>> now, though I'm not sure getting rid of async VM ops and replacing 
>> with a new way to directly wakeup the VMThread really amounts to a 
>> simplification.
>>
>> ---
>>
>> src/hotspot/share/runtime/vmOperations.hpp
>>
>> I still think getting rid of Mode altogether would be a good 
>> simplification. :)
> 
> Sure!
> 
> Here is v4, inc:
> http://cr.openjdk.java.net/~rehn/8234086/v4/inc/webrev/index.html
> Full:
> http://cr.openjdk.java.net/~rehn/8234086/v4/full/webrev/index.html
> 
> Tested t1-3
> 
> Thanks, Robbin
> 
> 
>>
>> Thanks,
>> David
>> -----
>>
>>
>>> Inc:
>>> http://cr.openjdk.java.net/~rehn/8234086/v3/inc/webrev/
>>>
>>> Tested t1-3
>>>
>>> Thanks, Robbin
>>>
>>> On 2019-11-19 12:05, Robbin Ehn wrote:
>>>> Hi all, please review.
>>>>
>>>> CMS was the last real user of the more advantage features of VM 
>>>> operation.
>>>> VM operation can be simplified to always be an stack object and thus 
>>>> either be
>>>> of safepoint or no safepoint type.
>>>>
>>>> VM_EnableBiasedLocking is executed once by watcher thread, if needed 
>>>> (default not used). Making it synchrone doesn't matter.
>>>> VM_ThreadStop is executed by a JavaThread, that thread should stop 
>>>> for the safepoint anyways, no real point in not stopping direct.
>>>> VM_ScavengeMonitors is only used to trigger a safepoint cleanup, the 
>>>> VM op is not needed. Arguably this thread should actually stop here, 
>>>> since we are about to safepoint.
>>>>
>>>> There is also a small cleanup in vmThread.cpp where an unused method 
>>>> is removed.
>>>> And the extra safepoint is removed:
>>>> "// We want to make sure that we get to a safepoint regularly"
>>>> No we don't :)
>>>>
>>>> Issue:
>>>> https://bugs.openjdk.java.net/browse/JDK-8234086
>>>> Change-set:
>>>> http://cr.openjdk.java.net/~rehn/8234086/v1/webrev/index.html
>>>>
>>>> Tested scavenge manually, passes t1-2.
>>>>
>>>> Thanks, Robbin