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

[coleen.phillimore at oracle.com]

http://cr.openjdk.java.net/~rehn/8234086/v5/full/webrev/src/hotspot/share/runtime/thread.cpp.udiff.html

With this change, can there be a further improvement to make 
VM_ThreadStop take Handles and remove oops_do() from VMOperation? (Maybe 
this is in the email thread ...)

Thanks,
Coleen

On 11/27/19 5:33 AM, David Holmes wrote:
> 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