RFR: 8183545: Event tracing, transition hooks

[Robbin Ehn]

Hi Dan,

On 07/05/2017 06:36 PM, Daniel D. Daugherty wrote:
> On 7/5/17 6:28 AM, Robbin Ehn wrote:
>> Hi all,
>>
>> Please review.
>>
>> This patch adds a new bit in suspend flag for the tracing backend.
>> Thus tracing backend can suspend threads on certain transitions.
>>
>> (David H and Dan D have looked at this, that's why they are on CC)
>>
>> I have also put in a oneliner in:
>> src/share/vm/classfile/javaClasses.cpp
>>
>> Which adds Threads_lock->owned_by_self() to the assert.
>> Threads_lock->owned_by_self() give the same guarantees as is_Watcher_thread(), e.g. there is no safepoint.
>>
>> If you really insist I can put the oneliner in a separate issue, but the tracing backend changes depends on both.
>>
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8183545
>> Webrev: http://cr.openjdk.java.net/~rehn/8183545/webrev/index.html
> 
> src/share/vm/classfile/javaClasses.cpp
>      Your comment about this change:
> 
>      > Threads_lock->owned_by_self() give the same guarantees as
>      > is_Watcher_thread(), e.g. there is no safepoint.
> 
>      confuses me. While it's true if the calling thread owns the
>      Threads_lock, we can't be in the middle of a safepoint, I
>      don't think that's the reason for the assert. Of course, if
>      the calling thread is the VMThread we could be in a safepoint,
>      but that was already allowed. I hate it when the reason for a
>      non-obvious assert is not commented...
> 
>      So the original code asserted:
> 
>      - WatcherThread allowed to call
>      - VMThread allowed to call
>      - JavaThread in state _thread_in_vm allowed to call
> 
>      You are adding:
> 
>      - Threads_lock owner allowed to call
> 
>      I suspect that the original reason for the assert() was to
>      make sure that the caller was doing something VM related
>      and your addition of the Threads_lock owner check fits that
>      criteria...
> 
>      A Mercurial history check shows:
> 
>         0: java_lang_Thread::ThreadStatus java_lang_Thread::get_thread_status(oop java_thread) {
>      4802:   assert(Thread::current()->is_Watcher_thread() || Thread::current()->is_VM_thread() ||
>         0:          JavaThread::current()->thread_state() == _thread_in_vm,
>         0:          "Java Thread is not running in vm");
> 
>      shows two of the lines come from the original import into
>      Mercurial (back in JDK7). The line you're changing was
>      changed since then:
> 
>      $ hg log -r 4802 src/share/vm/classfile/javaClasses.cpp
>      changeset:   4802:f2110083203d
>      user:        sla
>      date:        Mon Jun 10 11:30:51 2013 +0200
>      summary:     8005849: JEP 167: Event-Based JVM Tracing
> 
>      The line before changeset 4802 was:
> 
>      0:   assert(Thread::current()->is_VM_thread() ||
> 
>      $ hg diff -r 4801 -r 4802 src/share/vm/classfile/javaClasses.cpp
>      diff -r d0add7016434 -r f2110083203d src/share/vm/classfile/javaClasses.cpp
>      --- a/src/share/vm/classfile/javaClasses.cpp    Fri Jun 07 09:33:01 2013 -0700
>      +++ b/src/share/vm/classfile/javaClasses.cpp    Mon Jun 10 11:30:51 2013 +0200
>      @@ -961,7 +961,7 @@
> 
>       // Read thread status value from threadStatus field in java.lang.Thread java class.
>       java_lang_Thread::ThreadStatus java_lang_Thread::get_thread_status(oop java_thread) {
>      -  assert(Thread::current()->is_VM_thread() ||
>      +  assert(Thread::current()->is_Watcher_thread() || Thread::current()->is_VM_thread() ||
>                JavaThread::current()->thread_state() == _thread_in_vm,
>                "Java Thread is not running in vm");
>         // The threadStatus is only present starting in 1.5
> 
>      So JEP 167 added "Thread::current()->is_Watcher_thread()" and now
>      we want to add "Threads_lock->owned_by_self()"... can the previous
>      condition be removed or do we now need both?
> 
>      I'm wondering if adding a comment like this would make future
>      readings of this code more clear:
> 
>      // Make sure the caller is operating on behalf of the VM or is
>      // running VM code (state == _thread_in_vm).

Good digging, I think we can remove is_Watcher_thread from assert, get_thread_status is never called from Watcher thread.
(I'll run some tests also)

> 
> src/share/vm/runtime/thread.cpp
>      No comments.
> 
> src/share/vm/runtime/thread.hpp
>      L457:   bool is_trace_suspend()                 { return (_suspend_flags & _trace_flag) != 0; }
>          nit: indent for '{' is off by two space relative to the other
>               functions above and below it.
> 
>      L1269:     // We call is_external_suspend() last since external suspend should
>      L1270:     // be less common.
>      <snip>
>      L1279:     return (_special_runtime_exit_condition != _no_async_condition) ||
>      L1280:             is_external_suspend() || is_deopt_suspend() || is_trace_suspend();
>          Not your fault, but the quoted comment is wrong. I'm guessing
>          it has been wrong since is_deopt_suspend() was added to the
>          expression. Don't know if is_deopt_suspend() was added last
>          for a reason or not. I would think that is_deopt_suspend()
>          would be more frequent than external suspension.
> 
>          Looks like the s_deopt_suspend() conditional was added by this
>          Teamware delta:
> 
>          $ sp -r1.442.1.1 src/share/vm/runtime/thread.hpp
>          src/share/vm/runtime/SCCS/s.thread.hpp:
> 
>          D 1.442.1.1 06/12/07 10:06:52 sgoldman 1059 1057 00017/00003/01722
>          MRs:
>          COMMENTS:
>          6463133 - patchless deopt. Support specialized deopt suspend for
>          register window based machines.

Fixed!

> 
> src/share/vm/runtime/thread.inline.hpp
>      No comments.
> 
> src/share/vm/trace/traceMacros.hpp
>      No comments.
> 
> 
> Thumbs up on the change! If you fix the the nits above, I don't need
> to see a new webrev. I'm interested in what you decide to do about
> the src/share/vm/classfile/javaClasses.cpp assert, but that doesn't
> need to hold up this review.

Thanks!

/Robbin

> 
> Dan
> 
> 
>>
>> Test: openjdk build on Linux x64
>>
>> Thanks!
>>
>> /Robbin
>