RFR: 8212107: VMThread issues and cleanup [v2]

[David Holmes]

On 23/09/2020 9:27 pm, Robbin Ehn wrote:
> On Wed, 23 Sep 2020 11:02:54 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> inner_execute(..) is called in the non nested-case here:
>>> https://github.com/openjdk/jdk/blob/e49178a4445378fa0b5505ad6e9f1661636f88b8/src/hotspot/share/runtime/vmThread.cpp#L474
>>>
>>> Nested case:
>>> https://github.com/openjdk/jdk/blob/e49178a4445378fa0b5505ad6e9f1661636f88b8/src/hotspot/share/runtime/vmThread.cpp#L511
>>
>> Sorry I missed that. Seems odd that inner_execute handles nesting when that is only possible via one of the paths by
>> which it is called - that's why I thought it was only for the case where called from execute(). I'd rather see the
>> nesting logic handled as before, exclusively on the code path in which it can occur.
> 
> That would create a lot of code duplication:
> void VMThread::none_nested_inner_execute(VM_Operation* op) {
>    Thread* current = Thread::current();
>    assert(current->is_VM_thread(), "must be a VM thread");
> 
>    _cur_vm_operation = op;
> 
>    HandleMark hm(VMThread::vm_thread());
>    EventMark em("Executing %s VM operation: %s", op->name());
> 
>    // If we are at a safepoint we will evaluate all the operations that
>    // follow that also require a safepoint
>    log_debug(vmthread)("Evaluating %s %s VM operation: %s",
>                        _cur_vm_operation->evaluate_at_safepoint() ? "safepoint" : "non-safepoint",
>                        _cur_vm_operation->name());
> 
>    bool end_safepoint = false;
>    if (_cur_vm_operation->evaluate_at_safepoint()) {
>      SafepointSynchronize::begin();
>      if (_timeout_task != NULL) {
>        _timeout_task->arm();
>      }
>      end_safepoint = true;
>    }
> 
>    evaluate_operation(_cur_vm_operation);
> 
>    if (end_safepoint) {
>      if (_timeout_task != NULL) {
>        _timeout_task->disarm();
>      }
>      SafepointSynchronize::end();
>    }
> 
>    _cur_vm_operation = NULL;
> }
> Which 80% the same. (Same minus a few lines)

I envisaged simply moving the nesting check out of inner_execute and 
back into execute:

// psuedo-code
execute(VM_Operation* op) {
   if (on VMThread) {
     if (_cur_operation != NULL) {
       // nested case
       check_nesting_allowed();
       VM_Operation* prev = _cur_operation;
       _cur_operation = NULL;
       inner_execute(op);
       _cur_operation = prev;
     }
   }

Cheers,
David

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