RFR: 8212107: VMThread issues and cleanup [v2]

[David Holmes]

On Mon, 21 Sep 2020 07:33:27 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 with a new target base due to a merge or a rebase. The incremental webrev
> excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since
> the last revision:
>  - Merge branch 'master' into 8212107-vmthread
>  - Fixed nits
>  - Merge branch 'master' into 8212107-vmthread
>  - Fixes after review from shipilev.
>  - Fixed some indent misses
>  - Fixed ws
>  - Added assert
>  - Restructured and simplified
>  - Removed used linking in VM_Operation
>  - Removed ticket and use only one Monitor
>  - ... and 2 more: https://git.openjdk.java.net/jdk/compare/7c142951...e49178a4

This generally looks good. Mapping between the old way and the new way is a little tricky but I think I made all the
connections. One thing I did notice is that it seems that nested VM operations are now restricted to a nesting depth of
one - is that correct? (And the code could be a lot simpler if nesting was not needed. :) ). A couple of minor
comments/suggestions below. Thanks.
David

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

> 390:   EventMark em("Executing %s VM operation: %s", prev_vm_operation != NULL ? "nested" : "", op->name());
> 391:
> 392:   // If we are at a safepoint we will evaluate all the operations that

"all"? There can only be one surely?

src/hotspot/share/runtime/vmThread.hpp line 74:

> 72:
> 73:   void evaluate_operation(VM_Operation* op);
> 74:   void inner_execute(VM_Operation* op);

Perhaps nested_execute as it seems this is only for handling the case of a nested vm op?

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

Marked as reviewed by dholmes (Reviewer).

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