RFR(s): 8234086: VM operation can be simplified
David Holmes
david.holmes at oracle.com
Wed Nov 20 03:55:14 UTC 2019
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
More information about the hotspot-runtime-dev
mailing list