RFR: 8183545: Event tracing, transition hooks

[Daniel D. Daugherty]

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

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.

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.

Dan


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