RFR: 8212107: VMThread issues and cleanup

[Aleksey Shipilev]

On Thu, 17 Sep 2020 19:58:02 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.

I find juggling the `_next_vm_operation` a bit confusing at the first glance, but that seems superficially okay.

src/hotspot/share/runtime/mutexLocker.cpp line 68:

> 66: Mutex*   TouchedMethodLog_lock        = NULL;
> 67: Mutex*   RetData_lock                 = NULL;
> 68: Monitor* VMOperation_lock        = NULL;

Indenting is a bit off here?

src/hotspot/share/runtime/mutexLocker.hpp line 61:

> 59: extern Mutex*   TouchedMethodLog_lock;           // a lock on allocation of LogExecutedMethods info
> 60: extern Mutex*   RetData_lock;                    // a lock on installation of RetData inside method data
> 61: extern Monitor* VMOperation_lock;           // a lock on queue of vm_operations waiting to execute

Indenting is a bit off?

src/hotspot/share/runtime/thread.hpp line 625:

> 623:
> 624:   // For tracking the heavyweight monitor the thread is pending on.
> 625:   ObjectMonitor* current_pending_monitor() {

If this goes, then the declaration of `_vm_operation_started_count` can also go?

src/hotspot/share/runtime/thread.hpp line 627:

> 625:   ObjectMonitor* current_pending_monitor() {
> 626:     return _current_pending_monitor;
> 627:   }

Ditto for `_vm_operation_completed_count`?

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

> 220: void VMThread::wait_for_vm_thread_exit() {
> 221:   assert(Thread::current()->is_Java_thread(), "Should be a JavaThread");
> 222:   assert(((JavaThread*)Thread::current())->is_terminated(), "Should be terminated");

Looks like just `assert(JavaThread::current()->is_terminated(), "Should be terminated")`.

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

> 303:   // we emit a handshake if it's been more than a second since last.
> 304:   jlong deadline_ms = GuaranteedSafepointInterval != 0 ? GuaranteedSafepointInterval : 1000;
> 305:   deadline_ms += last_halot_ms;

It would seem more straight-forward to write it like this, to highlight the "base" `last_halot_ms` and the "addition"
`interval`:

 // ...
 jlong interval = (GuaranteedSafepointInterval != 0 ? GuaranteedSafepointInterval : 1000);
 jlong deadline_ms = last_halot_ms + interval;
 if (now_ms > deadline_ms) {
    last_halot_ms = now_ms;
    return true;
 }
 return false;

...but that's just a personal preference.

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

> 311: }
> 312:
> 313: void VMThread::cleanup_safepoint_alot() {

This handles `GuaranteedSafepointInterval` periodic op as well, so `cleanup_safepoint_alot` seems misnomer.

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

> 127:   // VM_Operation support
> 128:   static VM_Operation*     _cur_vm_operation;   // Current VM operation
> 129:   static VM_Operation*     _next_vm_operation;

Comment to match `// Current VM operation` above?

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

> 342: }
> 343:
> 344: void VMThread::until_executed(VM_Operation* op) {

...`until_executed`? Feels like missing a verb, e.g. `wait_until_executed`?

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

> 353:     }
> 354:     ml.notify_all();
> 355:     ml.wait();

It is really odd to see back-to-back `notify_all` and `wait`. Would that mean two threads sitting in this loop would
just wake one another continuously?

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

> 390:
> 391:   HandleMark hm(VMThread::vm_thread());
> 392:   EventMark em("Executing %s VM operation: %s", prev_vm_operation ? "nested" : "", op->name());

Check `prev_vm_operation != NULL` explicitly?

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

> 425:
> 426:   // Notify previous op done
> 427:   mu_queue.notify();

So, `notify`, not `notifyAll` here? We sure we want to bet who -- of all current waiters -- gets the notification?

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

> 468:   while (true) {
> 469:     wait_for_operation();
> 470:     if (!should_terminate()) {

This is a trailing block. So it might just be `do { ... } while (!should_terminate())`?

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

> 101:   // Returns the current vm operation if any.
> 102:   static VM_Operation* vm_operation()             {
> 103:     assert(Thread::current()->is_VM_thread(), "Must be);");

Typos in `Must be);`.

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

Changes requested by shade (Reviewer).

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