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

[Robbin Ehn]

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