RFR: 8212107: VMThread issues and cleanup [v6]

[Daniel D.Daugherty]

On Thu, 24 Sep 2020 06:27:46 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:
> 
>   Removed assert as suggested by David

Changes requested by dcubed (Reviewer).

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

> 314:   bool max_time_exceeded = GuaranteedSafepointInterval != 0 &&
> 315:                            (interval_ms >= GuaranteedSafepointInterval);
> 316:   if (!max_time_exceeded) {

You've changed the meaning of SafepointALot here. If max_time_exceeded
is false, then you never check the SafepointALot flag and miss causing a
safepointALot_op to happen next.

Here's the old code:

394   if (max_time_exceeded && SafepointSynchronize::is_cleanup_needed()) {
395     return &cleanup_op;
396   }
397   if (SafepointALot) {
398     return &safepointALot_op;
399   }

In the old code if max_time_exceeded and we need a cleanup,
then cleanup_op is the priority, but if that wasn't the case, then
we always checked the SafepointALot flag.

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

> 290: bool VMThread::handshake_alot() {
> 291:   assert(_cur_vm_operation == NULL, "Already have an op");
> 292:   assert(_next_vm_operation == NULL, "Already have an op");

These mesgs should be "should not have an op yet".

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

> 297:   jlong now_ms = nanos_to_millis(os::javaTimeNanos());
> 298:   // If only HandshakeALot is set, but GuaranteedSafepointInterval is 0,
> 299:   // we emit a handshake if it's been more than a second since last.

typo: s/since last/since the last one/

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

> 352:       // Wait to install this operation as the next operation in the VM Thread
> 353:       log_trace(vmthread)("A VM operation already set, waiting");
> 354:       ml.wait();

So instead of a thread enqueuing an operation on the VMop queue
and then waiting for the operation to be executed, we have the thread
waiting to enqueue the operation as the "next operation". It seems to
me that the new algorithm means that the waiting thread will be
woken up more often and then go back to wait()ing without progress.
Perhaps this is mitigated by there being way fewer VM operations in
the system, but I'm not sure.

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

> 359:     TraceTime timer("Waiting for VM operation to be completed", TRACETIME_LOG(Trace, vmthread));
> 360:     // _next_vm_operation is cleared holding VMOperation_lock after it have been
> 361:     // executed. We wait until _next_vm_operation not our op.

typo - s/it have been/it has been/
typo - s/not our op/is not our op/

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

> 361:     // executed. We wait until _next_vm_operation not our op.
> 362:     while (_next_vm_operation == op) {
> 363:       // VM Thread can process it once we unlocks the mutex on wait.

typo - s/we unlocks/we unlock/

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

> 382:   if (_cur_vm_operation != NULL) {
> 383:     // Check the VM operation allows nested VM operation.
> 384:     // This normally not the case, e.g., the compiler

typo - s/Check the VM/Check that the VM/
typo - s/This normally not/This is normally not/

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

> 385:     // does not allow nested scavenges or compiles.
> 386:     if (!_cur_vm_operation->allow_nested_vm_operations()) {
> 387:       fatal("Nested VM operation %s requested by operation %s",

clarity - s/Nested VM/Unexpected nested VM/

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

> 431:   // On first call this clears a dummy place-holder.
> 432:   _next_vm_operation = NULL;
> 433:   // Notify operation done and notify a next operation can be installed.

typo - s/operation done/operation is done/

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

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