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