RFR: 8212107: VMThread issues and cleanup [v3]

[Aleksey Shipilev]

On Wed, 23 Sep 2020 08:45:00 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> We simplify the vmThread by removing the queue and refactor the the main loop.
>> This solves the issues listed:
>> - It can create an extra safepoint directly after a safepoint.
>> - It's not safe for a non-JavaThread to add safepoint to queue while GC do oops do.
>> - The exposure of the vm operation is dangerous if it's a handshake.
>> - The code is a hornets nest with the repetition of checks and branches
>> 
>> Passes t1-8, and a benchmark run.
>> 
>> If you want a smaller diff the commits contains the incremental progress and each commit passed t1.
>
> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update after Coleen and David

I have only minor comments, without diving into the logic machinery. I am relying on others to review this more
substantially.

src/hotspot/share/runtime/vmThread.cpp line 389:

> 387:     if (!prev_vm_operation->allow_nested_vm_operations()) {
> 388:       fatal("Nested VM operation %s requested by operation %s",
> 389:             op->name(), vm_operation()->name());

This is better as `prev_vm_operation`, not `vm_operation()`? Saves some brain cycles reading...

src/hotspot/share/runtime/vmThread.cpp line 483:

> 481:       break;
> 482:     }
> 483:   } while(!should_terminate());

So now we have the duplicated `should_terminate()` here. Can we check it once (logically)? For example:

do {
  wait_for_operation();
  assert(_next_vm_operation != NULL, "Must have one");
  inner_execute(_next_vm_operation);
} while (!should_terminate());

If not, then it might be rewritten without change in logic this:

while (true) {
  if (should_terminate()) break;
  wait_for_operation();
  if (should_terminate()) break;
  assert(_next_vm_operation != NULL, "Must have one");
  inner_execute(_next_vm_operation);
}

src/hotspot/share/runtime/vmThread.hpp line 108:

> 106:   static VM_Operation::VMOp_Type vm_op_type()     {
> 107:     assert(Thread::current()->is_VM_thread(), "Must be");
> 108:     return _cur_vm_operation->type();

These two might just delegate and check a bit more thoroughly.

  static VM_Operation* vm_operation()             {
  static VM_Operation::VMOp_Type vm_op_type()     { return _cur_vm_operation->type(); }
  assert(Thread::current()->is_VM_thread(), "Must be");
    return _cur_vm_operation;
  }

  static VM_Operation::VMOp_Type vm_op_type()     {
    VM_Operation* op = vm_operation();
    assert (op != NULL, "sanity");
    return op->type();
  }

Haven't checked if `vm_op_type` helps readability at actual usages, maybe it is unnecessary to begin with.

src/hotspot/share/runtime/vmThread.cpp line 461:

> 459:     // We did find anything to execute, notify any waiter so they can install a new one.
> 460:     ml_op_lock.notify_all();
> 461:     ml_op_lock.wait(GuaranteedSafepointInterval);

I initially had the same concern as in my previous review: if there are multiple threads entering here, they could just
ping-pong each other with notify-wait. But then I realized this method is only called from VMThread itself. Maybe
methods like that need to be `assert(is_VM_thread())` on entry?

-------------

Marked as reviewed by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/228