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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Wed Nov 27 00:10:37 UTC 2019


Hi David,

Oh, right.

Thank you for explaining this!
Serguei

On 11/26/19 02:30, David Holmes wrote:
> Hi Serguei,
>
> Note that has_pending_exception() is not the same as having a pending 
> async exception. This is what Robbin was clarifying in his other mail. 
> The VM_StopThread sets the _pending_async_exception field, but that 
> exception only becomes the _pending_exception when we execute specific 
> thread-state transitions that check for the pending async exception - 
> and we apparently do not execute that kind of transition in this code. 
> Hence the assertion would not fire.
>
> Cheers,
> David
>
> On 26/11/2019 8:10 pm, serguei.spitsyn at oracle.com wrote:
>> Hi Robbin, Dan and David,
>>
>> Sorry for being slow with this reply.
>> Probably, it'd be Okay to reply just to this latest email from Robbin.
>>
>> I agree with Dan that we basically have the assert issue which Dan 
>> has spotted.
>> Calling the JVM TI StopThread on the target thread (current thread is 
>> target thread)
>> should cause the assertion to fire:
>>         src/hotspot/share/utilities/preserveException.cpp:
>>         assert(!_thread->has_pending_exception(), "unexpected 
>> exception generated");
>>
>> Below is just to make sure Robbin gets everything right ...
>>
>> ------------------------------------------------------------
>>
>> At build time we generate the jvmtiEnter.cpp with the JVM TI function 
>> wrappers
>> that check arguments and do state transitions.
>>
>> The location is like this:
>> build/linux-x86_64-server-release/hotspot/variant-server/gensrc/jvmtifiles/jvmtiEnter.cpp 
>>
>>
>> For the StopThread we have this wrapper:
>>
>> static jvmtiError JNICALL
>> jvmti_StopThread(jvmtiEnv* env,
>>              jthread thread,
>>              jobject exception) {
>>
>> #if !INCLUDE_JVMTI
>>    return JVMTI_ERROR_NOT_AVAILABLE;
>> #else
>>    if(!JvmtiEnv::is_vm_live()) {
>>      return JVMTI_ERROR_WRONG_PHASE;
>>    }
>>    Thread* this_thread = Thread::current_or_null();
>>    if (this_thread == NULL || !this_thread->is_Java_thread()) {
>>      return JVMTI_ERROR_UNATTACHED_THREAD;
>>    }
>>    JavaThread* current_thread = (JavaThread*)this_thread;
>>    ThreadInVMfromNative __tiv(current_thread);
>>    VM_ENTRY_BASE(jvmtiError, jvmti_StopThread , current_thread)
>>    debug_only(VMNativeEntryWrapper __vew;)
>> *  CautiouslyPreserveExceptionMark __em(this_thread);*
>>    JvmtiEnv* jvmti_env = JvmtiEnv::JvmtiEnv_from_jvmti_env(env);
>>    if (!jvmti_env->is_valid()) {
>>      return JVMTI_ERROR_INVALID_ENVIRONMENT;
>>    }
>>
>>    if (jvmti_env->get_capabilities()->can_signal_thread == 0) {
>>      return JVMTI_ERROR_MUST_POSSESS_CAPABILITY;
>>    }
>>    jvmtiError err;
>>    JavaThread* java_thread = NULL;
>>    ThreadsListHandle tlh(this_thread);
>>      err = JvmtiExport::cv_external_thread_to_JavaThread(tlh.list(), 
>> thread, &java_thread, NULL);
>>      if (err != JVMTI_ERROR_NONE) {
>>        return err;
>>      }
>>    err = jvmti_env->StopThread(java_thread, exception);
>>    return err;
>> #endif // INCLUDE_JVMTI
>> }
>>
>>
>> The VM_ThreadStop::doit() calls this:
>>       target->send_thread_stop(throwable());
>>
>> which sets a pending async. exception in the target thread (which is 
>> current thread in our case):
>>        // Set async. pending exception in thread.
>>        set_pending_async_exception(java_throwable);
>>
>>
>> AFAIK the VM_ThreadStop is executed at a safepoint (not 
>> concurrently), so that it should cause
>> to fire the below assert in the CautiouslyPreserveExceptionMark 
>> desctructor:
>>
>> CautiouslyPreserveExceptionMark::~CautiouslyPreserveExceptionMark() {
>> *  assert(!_thread->has_pending_exception(), "unexpected exception 
>> generated");*
>>    if (_thread->has_pending_exception()) {
>>      _thread->clear_pending_exception();
>>    }
>>    if (_preserved_exception_oop() != NULL) {
>> _thread->set_pending_exception(_preserved_exception_oop(), 
>> _preserved_exception_file, _preserved_exception_line);
>>    }
>> }
>> ------------------------------------------------------------------
>>
>>
>> I'm not sure we have a test coverage for this case.
>> It looks like the JVM TI StopThread was not designed to stop the 
>> current thread.
>> I think so because the NULL is not accepted as thread parameter to 
>> designate current thread.
>> The error JVMTI_ERROR_INVALID_THREAD has to be returned in such a case.
>> But it is hard to say why this assumption was not spelled clearly in 
>> the spec.
>>
>> Overall, it does not look important to keep the StopThread correctly 
>> working for target thread == current.
>> It is because there is always an option to send a synchronous 
>> exception to itself.
>>
>> I don't know why this was not deprecated together with the 
>> Thread.stop().
>> The JVM TI SuspendThread()/ResumeThread() also were not deprecated 
>> together with Thread.suspend()/Thread.resume().
>> I think, these functions are needed for debuggers.
>>
>> Thanks,
>> Serguei
>>
>>
>>
>>
>> On 11/25/19 04:48, Robbin Ehn wrote:
>>> Hi Serguei, thanks for having a look.
>>>
>>> AFAIK:
>>> Today one of these three can happen when returning to agent (native).
>>> - The target thread for stop have not yet installed the async 
>>> exception.
>>> - The target thread have installed the async exception, but not yet 
>>> stopped.
>>> - The target thread have installed the async exception and already 
>>> stopped.
>>>
>>> A agent must handle all three possible scenarios.
>>> This patch just removes the first scenario, and makes the 
>>> installation part synchrone.
>>>
>>> Hope that helps!
>>>
>>> Note, I don't see the method "StopThread" being documented as either 
>>> synchrone
>>> or asynchrone itself.
>>> The exception is documented as beeing asynchrone.
>>>
>>> And I don't think we follow the specs now, since we ignore the 
>>> result of:
>>> void VM_ThreadStop::doit()
>>> One would expect e.g. JVMTI_ERROR_THREAD_NOT_ALIVE if we never 
>>> deilver the async
>>> exception. So making the async exception installation part synchrone 
>>> would be a
>>> step to fix that issue.
>>>
>>> Thanks, Robbin
>>>
>>> On 2019-11-25 12:45, serguei.spitsyn at oracle.com wrote:
>>>> Please, skip my reply below.
>>>> I need to read all emails carefully.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 11/25/19 03:35, serguei.spitsyn at oracle.com wrote:
>>>>> 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