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

[David Holmes]

On 27/11/2019 8:09 pm, Robbin Ehn wrote:
> Thanks David!
> 
> I hope you noted that there is a v5 also  with Dan's comments:
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-November/037212.html 
> 
> http://cr.openjdk.java.net/~rehn/8234086/v5/inc

I hadn't noticed. Those changes are also fine. The use of xchg rather 
than cmpxchg is a good simplification given the value must be 0 or 1.

> To many mails here :)

:) Makes me wonder how things will work once we move to git ;-)

Thanks,
David

> 
> /Robbin
> 
> On 11/27/19 2:34 AM, David Holmes wrote:
>> 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