RFR(s): 8234086: VM operation can be simplified
Robbin Ehn
robbin.ehn at oracle.com
Wed Nov 20 12:57:21 UTC 2019
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
More information about the hotspot-runtime-dev
mailing list