RFR(s): 8234086: VM operation can be simplified
Robbin Ehn
robbin.ehn at oracle.com
Mon Nov 25 08:06:50 UTC 2019
Hi,
Starting with this email due to thanksgiving.
On 2019-11-25 06:36, David Holmes wrote:
> Hi Dan,
>
> I support your analysis regarding JVM TI StopThread. It's very hard via code
> inspection to be 100% certain but I think Robbin's change will install the
> async-exception in the current thread in the context of the StopThread call,
> resulting in the CautiouslyPreserveExceptionMark asserting. Unfortunately JVM TI
> StopThread doesn't special-case the current thread the way JVM_StopThread does.
It passes jvmti/jdi tests.
The async-exception is thread internally only delivered when going to java, via
the suspend flags. In the case going VM->native it should not be delivered.
I'll investigate and have a look.
I find it very unsettling that jvmti StopThread is not deprecated?
This have exactly the same flaws as Thread.stop().
Meaning even if we remove Thread.stop() the VM needs to support this flawed
stopping ability...
>
> Your observations about the WatcherThread change in behaviour are also spot on.
> Potentially at least, forcing the WatcherThread to wait for the safepoint to be
> executed could interfere with executing other periodic tasks. By default the
> WatcherThread won't be executing this code as the BiasedLockingStartupDelay is
> zero. But potentially, if anyone has that delay enabled, this could cause an
> observable change in behaviour in relation to other PeriodicTasks.
In RFR I had this comment:
VM_EnableBiasedLocking is executed once by watcher thread, if needed (default
not used). Making it synchrone doesn't matter.
A periodic task have a minimum resolution of 10ms, while the safepoint for
enabling biased locking takes <1ms under normal circumstances. On an
over-provisioned machine we see longer safepoints, but we see also see scheduler
delays up to 35-40ms.
I deemed it very unlikely that it is possible to notice it.
>
> Perhaps the ability to execute an async-safepoint VM operation needs to remain,
> for simplicity (compared to working around the issues).
I'm hoping not :(
/Robbin
>
> David
> -----
>
> On 23/11/2019 7:50 am, Daniel D. Daugherty wrote:
>> Hi Robbin,
>>
>> Sorry I'm late to this review thread...
>>
>> I'm adding Serguei to this email thread since I'm making comments
>> about the JVM/TI parts of this changeset...
>>
>>
>>> http://cr.openjdk.java.net/~rehn/8234086/v4/full/webrev/index.html
>>
>>
>> src/hotspot/share/runtime/vmOperations.hpp
>> No comments.
>>
>> src/hotspot/share/runtime/vmOperations.cpp
>> No comments.
>>
>> src/hotspot/share/runtime/vmThread.hpp
>> L148: // The ever running loop for the VMThread
>> L149: void loop();
>> L150: static void check_cleanup();
>> nit - Feels like an odd place to add check_cleanup().
>>
>> Update: Now that I've seen what clean_up(), it needs a
>> better name. Perhaps check_for_forced_cleanup()? And since
>> it is supposed to affect the running loop for the VMThread
>> I'm okay with its location now.
>>
>> src/hotspot/share/runtime/vmThread.cpp
>> L382: event->set_blocking(true);
>> Probably have to keep the 'blocking' attribute in the event
>> for backward compatibility in the JFR record format?
>>
>> L478: // wait with a timeout to guarantee safepoints at regular
>> intervals
>> Is this comment true anymore (even before this changeset)?
>> Adding this on the next line might help:
>>
>> // (if there is cleanup work to do)
>>
>> since I _think_ that's how the policy has been evolved...
>>
>> L479: mu_queue.wait(GuaranteedSafepointInterval);
>> Please prefix with "(void)" to make it clear you are
>> intentionally ignoring the return value.
>>
>> old L627-634 (We want to make sure that we get to a safepoint regularly)
>> I think this now old code is covered by your change above:
>>
>> L488: // If the queue contains a safepoint VM op,
>> L489: // clean up will be done so we can skip this part.
>> L490: if (!_vm_queue->peek_at_safepoint_priority()) {
>>
>> Please confirm that our thinking is the same here.
>>
>> L661: int ticket = t->vm_operation_ticket();
>> nit - extra space after '='
>>
>> Okay. Definitely simpler code.
>>
>> src/hotspot/share/runtime/handshake.cpp
>> No comments.
>>
>> src/hotspot/share/runtime/safepoint.hpp
>> No comments.
>>
>> src/hotspot/share/runtime/safepoint.cpp
>> Definitely got my attention with
>> ObjectSynchronizer::needs_monitor_scavenge().
>>
>> src/hotspot/share/runtime/synchronizer.hpp
>> No comments.
>>
>> src/hotspot/share/runtime/synchronizer.cpp
>> L921: log_info(monitorinflation)("Monitor scavenge needed, triggering
>> safepoint cleanup.");
>> Thanks for adding the logging line.
>>
>> Update: As Kim pointed out, this code goes away when
>> MonitorBound is made obsolete (JDK-8230940). I'm looking
>> forward to making that change.
>>
>> L1003: if (Atomic::load(&_forceMonitorScavenge) == 0 && Atomic::xchg
>> (1, &_forceMonitorScavenge) == 0) {
>> nit - extra space between 'xchg ('
>>
>> Since InduceScavenge() is only called when the deprecated
>> MonitorBound is specified, I think you could use cmpxchg()
>> for clarity. Of course, you might be thinking that the
>> pattern is a useful example for other folks to copy...
>>
>> src/hotspot/share/runtime/thread.cpp
>> old L527: // Enqueue a VM_Operation to do the job for us - sometime later
>> L527: void Thread::send_async_exception(oop java_thread, oop
>> java_throwable) {
>> L528: VM_ThreadStop vm_stop(java_thread, java_throwable);
>> L529: VMThread::execute(&vm_stop);
>> L530: }
>> Okay so you deleted the comment about the call being async and the
>> VM op is no longer async, but does that break the expectation of
>> any callers?
>>
>> Off the top of head, I can't think of a way for a caller of
>> Thread::send_async_exception() to determine that the call is now
>> synchronous instead of asynchronous, but ...
>>
>> Update: Just took a look at JvmtiEnv::StopThread() which calls
>> Thread::send_async_exception(). If JVM/TI StopThread() is being
>> used to throw an exception at the calling thread, I suspect that
>> in the baseline, the call would always return JVMTI_ERROR_NONE.
>> With the exception throwing now being synchronous, would that
>> affect the return value of the JVM/TI StopThread() call?
>>
>> Looks like the JVM/TI wrapper (see gensrc/jvmtifiles/jvmtiEnter.cpp
>> in the build directory) uses ThreadInVMfromNative so the calling
>> thread is in VM when it requests the now synchronous VM operation.
>> When it requests the VM op, the calling thread will block which
>> should allow the VM thread to execute the op. No worries there so
>> far...
>>
>> It looks like the code also uses CautiouslyPreserveExceptionMark
>> so I think if the exception is delivered to the calling thread
>> it won't affect the return from jvmti_env->StopThread(), i.e., we
>> will have our return value. The CautiouslyPreserveExceptionMark
>> destructor won't kick in until we return from jvmti_StopThread()
>> (the JVM/TI wrapper from the build).
>>
>> However, that might cause this assertion to fire:
>>
>> src/hotspot/share/utilities/preserveException.cpp:
>> assert(!_thread->has_pending_exception(), "unexpected exception
>> generated");
>>
>> because it is now detecting that an exception was thrown
>> while executing a JVM/TI call. This is pure theory here.
>>
>> src/hotspot/share/jfr/leakprofiler/utilities/vmOperation.hpp
>> No comments.
>>
>> src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp
>> No comments.
>>
>> src/hotspot/share/runtime/biasedLocking.cpp
>> old L85: // Use async VM operation to avoid blocking the Watcher thread.
>> Again, you've deleted the comment, but is there going to
>> be any unexpected side effects from the change? Looks like
>> the work consists of:
>>
>> L70: ClassLoaderDataGraph::dictionary_classes_do(enable_biased_locking);
>>
>> Is that going to be a problem for the WatcherThread?
>>
>> test/hotspot/gtest/threadHelper.inline.hpp
>> No comments.
>>
>> As David H. likes to say: the proof is in the building and testing.
>>
>> Thumbs up on the overall idea and implementation. There might be an
>> issue lurking there in JVM/TI StopThread(), but that's just a theory
>> on my part...
>>
>> Dan
>>
>>
>>
>> On 11/22/19 9:39 AM, Robbin Ehn wrote:
>>> 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
>>
More information about the hotspot-runtime-dev
mailing list