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

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Nov 22 21:58:44 UTC 2019


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].

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...

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