RFR(s): 8234086: VM operation can be simplified
David Holmes
david.holmes at oracle.com
Tue Nov 26 10:30:20 UTC 2019
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