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

[Robbin Ehn]

Hi David,

On 11/20/19 4:55 AM, David Holmes wrote:
> 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?

Yes, exactly.

> 
> 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.
> 

Thanks, removed!

> ---
> 
> 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. ??

To keep the same behavior as an asynch safepoint I did this.
Kim also didn't like this, suggestions?
(I did not want to block the thread here)

> 
> ---
> 
> 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 head was heap allocated.
By having the heads static instead we don't need to allocate them.

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

I have a patch for that, but is in a bigger change-set which also removes oops
do and thus the drain queue is not needed. But to actually do that change
sanely, I had to refactor a lot, which makes it large change.
So I'm saving that for JDK 15...

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

Fixed.

> 
> -         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.

Yes, thanks, this is not the code I intended.
Fixing...

> 
> 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!

(sending v3 after testing to RFR mail)

/Robbin

> 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