RFR: 8212107: VMThread issues and cleanup [v3]

[Robbin Ehn]

On Wed, 23 Sep 2020 13:00:43 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update after Coleen and David
>
> src/hotspot/share/runtime/vmThread.hpp line 108:
> 
>> 106:   static VM_Operation::VMOp_Type vm_op_type()     {
>> 107:     assert(Thread::current()->is_VM_thread(), "Must be");
>> 108:     return _cur_vm_operation->type();
> 
> These two might just delegate and check a bit more thoroughly.
> 
>   static VM_Operation* vm_operation()             {
>   static VM_Operation::VMOp_Type vm_op_type()     { return _cur_vm_operation->type(); }
>   assert(Thread::current()->is_VM_thread(), "Must be");
>     return _cur_vm_operation;
>   }
> 
>   static VM_Operation::VMOp_Type vm_op_type()     {
>     VM_Operation* op = vm_operation();
>     assert (op != NULL, "sanity");
>     return op->type();
>   }
> 
> Haven't checked if `vm_op_type` helps readability at actual usages, maybe it is unnecessary to begin with.

vm_operation() may return NULL as callers handles this, as long it's the VM Thread calling a non NULL is also fine.
But we should remove this method and do something else, but for another PR :)

vm_op_type() is only used at one place, but I think your version is much better so I took that!

> src/hotspot/share/runtime/vmThread.cpp line 461:
> 
>> 459:     // We did find anything to execute, notify any waiter so they can install a new one.
>> 460:     ml_op_lock.notify_all();
>> 461:     ml_op_lock.wait(GuaranteedSafepointInterval);
> 
> I initially had the same concern as in my previous review: if there are multiple threads entering here, they could just
> ping-pong each other with notify-wait. But then I realized this method is only called from VMThread itself. Maybe
> methods like that need to be `assert(is_VM_thread())` on entry?

Adding assert(s)!

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

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