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

Robbin Ehn robbin.ehn at oracle.com
Mon Nov 25 08:33:07 UTC 2019


Hi Dan,

On 2019-11-22 22:58, Daniel D. Daugherty wrote:
> Just to you...
> 
> I think you need to do at least one set of Mach5 runs that includes the
> higher tiers. With this change, you really need JPDA (including JVM/TI)
> which comes in at Tier5 and you need stress testing which comes in at
> Tiers[678].

This have been tested together with a bigger change-set t1-5 and nsk jdi/jvmti.

> 
> I tend to submit Tier[1-3], Tier[4-6], Tier7 and then Tier8. This gets
> thru more quickly than a Tier[1-8] which has too many tasks...

I'll do that.

Thanks, Robbin

> 
> Dan
> 
> 
> On 11/22/19 4:50 PM, 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