RFR: 8212107: VMThread issues and cleanup [v6]

[Robbin Ehn]

On Thu, 24 Sep 2020 20:19:08 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Removed assert as suggested by David
>
> 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".

Fixed

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

Fixed

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

The old behavior could create a SafepointALot when we had no 'safepoint priority' ops in queue when woken.
To get this behavior we need more logic to avoid back to back SafepointALot and we need to peek at _next_vm_operation
to determine if it's a safepoint op or not (handshake).

During a normal test run the old behavior only creates around 1% more safepoints.
And if you want more safepoints you can decrease GuaranteedSafepointInterval (not exactly the same).

So I didn't think adding that complexity to exactly mimic the old behavior was worth it.

What you want me to do?

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

Fixed

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

Fixed

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

Fixed

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

Fixed

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

Fixed

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

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