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

Fri Nov 22 21:50:38 UTC 2019

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