RFR(s): 8234086: VM operation can be simplified
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Nov 25 11:35:17 UTC 2019
Hi Dan and Robbin,
I can be wrong and missing something but it feels like there is no issue
for JVMTI with this fix.
> 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 ...
There can be some confusion here about what is synchronous relative to.
I read it this way:
It synchronous for the current thread which calls the
send_async_exception().
However, it is asynchronous for the target thread that needs to be
stopped.
So that the fix does not break the JVMTI spec requirements.
Please, let me know if you agree (or not) with this reading.
Thanks,
Serguei
On 11/22/19 13:50, 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