RFR(s): 8223313: Use handshakes for CountStackFrames.

[Daniel D. Daugherty]

On 5/15/19 7:17 AM, Robbin Ehn wrote:
> Hi all please review.
>
> This change is part of parent issue:
> Utilize handshakes instead of is_thread_fully_suspended
> https://bugs.openjdk.java.net/browse/JDK-8223312
>
> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8223313
> Change-set:
> http://cr.openjdk.java.net/~rehn/8223313/webrev/

src/hotspot/share/prims/jvm.cpp
     old L3019:     if (receiver->is_thread_fully_suspended(true /* wait 
for suspend completion */, &debug_bits)) {
     new L3016:     if (!jt->is_external_suspend()) {
         is_external_suspend() simply checks to see if an external
         suspension has been requested for the thread:

         src/hotspot/share/runtime/thread.hpp
           bool is_external_suspend() const {
             return (_suspend_flags & _external_suspend) != 0;
           }

         It says absolutely nothing about whether the target thread is
         actually suspended (and its stack is safe to walk).

         The "is_thread_fully_suspended(true /* wait for suspend 
completion */"
         call checks to see if the thread has completed the external
         suspension protocol. This is a necessary step for cooperative
         self-suspension.

         However, you've replaced cooperative self-suspension for this
         case with a handshake so the handshake from the target thread
         only happens when that thread's stack can be safely walked.
         This is nice!

         I agree with replacing the "is_thread_fully_suspended(true,"
         call here with is_external_suspend(). A properly coded use of
         Thread suspension followed by a JVM_CountStackFrames() call
         will not be able to detect any difference. If you have a case
         with a racing/rogue Thread resume, your new handshake based
         solution will return the proper (but soon to be stale) answer
         but it won't crash. With "is_thread_fully_suspended(true,"
         there is the possibility of a crash during the stack walk
         when there is a rogue/racing resume (which is one of the
         reasons for deprecation).

So a very long way of saying: Thumbs up!

Dan


A bit more commentary:

https://docs.oracle.com/en/java/javase/12/docs/api/java.base/java/lang/Thread.html#countStackFrames()
     countStackFrames() is not only deprecated, but it is deprecated for
     removal. That's the really good news here! However, deprecation does
     not mean that the API cannot be used. It means we recommend that you
     don't use it and if you have issues with the API, then we'll remind
     you that it is deprecated and you should not use it.

And a clarification:
     The reason for the "is_thread_fully_suspended(true," call is
     that a call to SuspendThread() will return to the caller once
     an external suspend is requested. The target thread does not
     have to have completed the self-suspension, but it does have
     to not execute any more bytecode or bytecode equivalents.

     So this sequence:

         SuspendThread(target);
         CountStackFrames(target);

     requires that CountStackFrames() wait for target's stack to
     be safe to walk. In the cooperative self-suspension world,
     that means waiting for the suspend request to be complete.
     Without "is_thread_fully_suspended(true," (in the current
     system), it is possible for CountStackFrames() to crash
     even without a rogue resume.

>
> The only test using this seems to be CountStackFramesAtExit.java which 
> passes fine.
>
> Thanks, Robbin