RFR: 8212107: VMThread issues and cleanup

[Robbin Ehn]

On Fri, 18 Sep 2020 05:04:30 GMT, Aleksey Shipilev <shade 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.
>
> 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?

Fixing

> 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?

Fixing

> 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?

Fixing

> 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`?

Fixing

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

Fixing

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

Fixing

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

Fixing, but not sure if you like.

> 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`?

Fixing

> 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?

Fixing

> 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?

Will be changed, due to above issue.

> 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())`?

Fixing

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

Fixing

> 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?

Fixing

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

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