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

[David Holmes]

Hi Robbin,

Meta-comment: I found it difficult to identify exactly what was being 
removed/simplified. Please update the bug report with basic information 
on what you've actually done here: always stack allocated; no concurrent 
ops; no async ops.

Meta-comment2: I would have thought that concurrent handshake ops might 
be useful/desirable - though I assume your aim is to make such ops 
actually bypass the VMThread altogether in the future?

On 19/11/2019 11:51 pm, Robbin Ehn wrote:
> Hi David, thanks for having a look.
> 
> On 11/19/19 2:27 PM, David Holmes wrote:
>> Hi Robbin,
>>
>> On 19/11/2019 9:05 pm, Robbin Ehn wrote:
>>> Hi all, please review.
>>
>> Preliminary comments ... so ...
>>
>> +class VM_Operation : public StackObj {
>>    public:
>>     enum Mode {
>>       _safepoint,       // blocking,        safepoint, vm_op C-heap 
>> allocated
>> -    _no_safepoint,    // blocking,     no safepoint, vm_op C-Heap 
>> allocated
>> -    _concurrent,      // non-blocking, no safepoint, vm_op C-Heap 
>> allocated
>> -    _async_safepoint  // non-blocking,    safepoint, vm_op C-Heap 
>> allocated
>> +    _no_safepoint    // blocking,     no safepoint, vm_op C-Heap 
>> allocated
>>     };
>>
>>
>> You are basically getting rid of concurrent and async_safepoint VM op 
>> capability. Okay. But you're also making all VM ops StackObj so all 
>> those "VM op C-heap allocated" comments are no longer correct. Also 
>> many of the comments around the VM ops you have changed from async to 
>> sync,and from C-heap to stackobj, are also no longer correct.
>>
>> Please update.
> 
> Not sure if I found all:
> http://cr.openjdk.java.net/~rehn/8234086/v2/inc/webrev/index.html
> http://cr.openjdk.java.net/~rehn/8234086/v2/full/webrev/index.html

As per Kim's email no you didn't spot them all :)

Plus:

src/hotspot/share/runtime/thread.cpp

  // Enqueue a VM_Operation to do the job for us - sometime later

The "sometime later" is no longer applicable as it isn't async.

---

I'm really not seeing what the 
ObjectSynchronizer::force_monitor_scavage() related changes are all 
about. They don't seem to be part of the simplification but seem to be a 
separate issue. ??

---

src/hotspot/share/runtime/vmOperations.cpp/hpp

If there are only safepoint and no-safepoint ops now then we don't need 
a Mode enum just a virtual evaluate_at_safepoint() query.

Not sure how adding VM_QueueHead simplifies anything ??

The real simplification with this would come with getting rid of 
priorities :)

Minor nit: If there is no queue_add_front then queue_add_back could just 
be queue_add.

-         if (timedout) {
+         {

Not at all clear why the forced safepoint code is now unconditional. I 
see we check the interval more explicitly in VMThread::no_op_safepoint, 
but it isn't clear that we always want to waste time unlocking and 
relocking the lock unnecessarily.

The timedout local variable is also now unused.

  648     // only blocking VM operations need to verify the caller's 
safepoint state:

All ops are now blocking.

Thanks,
David


> Thanks, Robbin
> 
>>
>> Thanks,
>> David
>> -----
>>
>>> 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