RFR: 8212107: VMThread issues and cleanup [v2]

[Coleen Phillimore]

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/6c757159...e49178a4

Looks like a nice cleanup. I had a couple of questions.

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

> 50: #include "utilities/events.hpp"
> 51: #include "utilities/vmError.hpp"
> 52: #include "utilities/xmlstream.hpp"

Since you've changed a lot of the body of this, can you make sure that all these header files are needed?  method.hpp,
runtimeService.hpp and dtrace.hpp may no longer be needed.  Check with precompiled headers off though.

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

> 349:     if (VMThread::vm_thread()->set_next_operation(op)) {
> 350:       ml.notify_all();
> 351:       break;

I was wondering if you should put some counter while the thread is waiting to install its operation for logging and
debugging purposes?  Since the supposition is that there are few operations that cause safepoints so there's never a
long wait.

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

> 350:       ml.notify_all();
> 351:       break;
> 352:     }

// Wait to install this operation as the next operation in the VM Thread

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

> 355:   // _next_vm_operation is cleared holding VMOperation_lock
> 356:   // after it have been executed.
> 357:   while (_next_vm_operation == op) {

// Wait until the operation has been processed

But can this operation still be being processed as the current process?

Anyway can you add a couple of comments to help the reader.

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

Marked as reviewed by coleenp (Reviewer).

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