RFR(s): 8220774: Add HandshakeALot diag option

[Daniel D. Daugherty]

On 3/19/19 12:48 PM, Robbin Ehn wrote:
> Hi all, please review.
>
> Changeset:
> http://cr.openjdk.java.net/~rehn/8220774/webrev/

src/hotspot/share/runtime/globals.hpp
     No comments.

src/hotspot/share/runtime/thread.hpp
     L1871:   // Debug method asserting thread states
     L1872:   DEBUG_ONLY(void assert_states();)
         The comment doesn't add anything beyond what you can see with
         the line of code (except for the "thread" part). I think you
         need a better function name and a better comment.

src/hotspot/share/runtime/thread.cpp
     L2952: void JavaThread::assert_states() {
     L2953:   assert((!has_last_Java_frame() && java_call_counter() == 0) ||
     L2954:          (has_last_Java_frame() && java_call_counter() > 0), 
"wrong java_sp info!");
         The assert failure string doesn't really match the assertion
         unless I missing something here...

         Also what's the reason for this assertion?

src/hotspot/share/runtime/vmThread.hpp
     No comments.

src/hotspot/share/runtime/vmThread.cpp
     L437: class HandshakeALotTC : public ThreadClosure {
     <snip>
     L439:   virtual void do_thread(Thread* thread) {
     <snip>
     L441:     JavaThread* jt = (JavaThread*)thread;
     L442:     DEBUG_ONLY(jt->assert_states();)
         Okay, but why are these thread states important here?

         Also L441 could also be DEBUG_ONLY. In fact the whole function
         body could be just #ifdef ASSERT ... #endif.

     old L437: VM_Operation* VMThread::no_op_safepoint(bool check_time) {
     <snip>
     old L444:   if (check_time) {
     old L445:     long interval_ms = 
SafepointTracing::time_since_last_safepoint_ms();
     old L446:     bool max_time_exceeded = GuaranteedSafepointInterval 
!= 0 &&
     old L447:                              (interval_ms > 
GuaranteedSafepointInterval);
     old L448:     if (!max_time_exceeded) {
     old L449:       return NULL;
     old L450:     }
     old L451:   }
         In the existing code base, all callers to no_op_safepoint()
         passed 'false' so L444-L451 was dead code. It looks like
         before this changeset, we had both 'true' and 'false' callers:

         changeset:   53895:b22d8ae270a2
         user:        rehn
         date:        Fri Feb 22 14:20:06 2019 +0100
         summary:     8219436: Safepoint logs correction and misc

     new L446: VM_Operation* VMThread::no_op_safepoint() {
     new L447:   long interval_ms = 
SafepointTracing::time_since_last_safepoint_ms();
     new L448:   bool max_time_exceeded = GuaranteedSafepointInterval != 
0 &&
     new L449:                            (interval_ms >= 
GuaranteedSafepointInterval);
     new L450:   if (!max_time_exceeded) {
     new L451:     return NULL;
     new L452:   }
         In the new code, L447-L452 are no longer dead and in fact the
         block is now first and is always checked. This means that if
         max_time_exceeded == false, we return NULL and there's no
         safepoint.

         This is a change in behavior for the SafepointALot flag since
         this code block used to be first:

         old L438:   if (SafepointALot) {
         old L439:     return &safepointALot_op;
         old L440:   }

         So unless I'm missing something SafepointALot now appears to
         be broken since it will only happen if max_time_exceeded == true
         and that's just a normal safepoint interval.

     old L494:         if (timedout && (_cur_vm_operation = 
VMThread::no_op_safepoint(false)) != NULL) {
     old L495:           MutexUnlockerEx mul(VMOperationQueue_lock,
     old L496: Mutex::_no_safepoint_check_flag);
     new L506:           MutexUnlockerEx mul(VMOperationQueue_lock, 
Mutex::_no_safepoint_check_flag);
     new L507:           if (timedout && (_cur_vm_operation = 
VMThread::no_op_safepoint()) != NULL) {
         The old code only unlocked (and relocked) VMOperationQueue_lock
         when we knew that we needed to force a safepoint. Now it is always
         unlocked (and relocked) on this code path.

     old L621:     if ((_cur_vm_operation = 
VMThread::no_op_safepoint(false)) != NULL) {
     new L633:     if ((_cur_vm_operation = VMThread::no_op_safepoint()) 
!= NULL) {
         Before the fix for 8219436, this line used to be:

             if (VMThread::no_op_safepoint_needed(true)) {

         so the fix for 8219436 is what made old L444-L451 into
         dead code.


Okay, so I think that this fix breaks the SafepointALot flag.

The fix for 8219436 broke the max_time_exceeded logic and
this fix solves that, but it breaks SafepointALot at the same
time so I think you need to rework things a bit.

Dan


> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8220774
>
> We need an option to generate a lot of handshakes for testing.
> This adds -XX:+HandshakeALot, typical usage:
> -XX:+UnlockDiagnosticVMOptions -XX:GuaranteedSafepointInterval=200 
> -XX:+HandshakeALot
> Will generate a extra handshake if there is no safepoint/handshake per 
> 200ms.
>
> Thanks, Robbin
>