RFR: 8254758: Change G1ServiceThread to be task based [v3]

[Albert Mingkun Yang]

On Tue, 20 Oct 2020 10:17:30 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> Please review this enhancement to make `G1ServiceThread` task based. This thread has evolved from having a single
>> responsibility to do more things. And there are plans to add more to it. To make this easier and to allow different
>> tasks to run at different intervals this change adds a simple task mechanism to the service thread.  The idea is that
>> the service thread keeps a list of `G1ServiceTask`-objects that are ordered by when they should execute. When no tasks
>> are ready to be run the service thread check how far into the future the next task is scheduled and sleeps/waits for
>> that duration. If a new task is added while the thread is waiting it will be woken up to check for ready tasks.   There
>> are currently two tasks in the list. They have been extracted from the old service thread into the tasks:
>> `G1PeriodicGCTask` and `G1RemSetSamplingTask`. These are more or less identical to the old code, but in the form of
>> tasks. The only difference is that `G1PeriodicGCTask` no longer need to keep track of the last attempt, since it will
>> be rescheduled with the correct interval.   To reschedule a task is to update the time when it should be executed next
>> and adding it back to the list. This insertion is O(n) and it should be fine since the expected number of tasks handled
>> by the service thread is low. If a task don't want to be rescheduled it can set a negative timeout and that will cause
>> the task to be dropped from the list. There are currently no such tasks and adding such tasks come with the
>> responsibility of making sure resources are taken care of when the task is no longer active.  I've added a gtest to do
>> some basic VM-testing and apart from a lot of local testing I've run mach5 tier 1-4 a few times with good results.  I
>> have created a few follow up tasks to look at moving the current tasks to more appropriate locations and also updated a
>> few old service thread related tasks with the label:
>> [`gc-g1-service-thread`](https://bugs.openjdk.java.net/issues/?jql=labels%20%3D%20gc-g1-service-thread)
>
> Stefan Johansson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Ivan and Albert review

src/hotspot/share/gc/g1/g1ServiceThread.cpp line 259:

> 257:
> 258:   // Reschedule task by updating task time and add back to queue.
> 259:   double delay_ms = task->delay_ms() / 1000.0;

The delay is in seconds now, right?

src/hotspot/share/gc/g1/g1ServiceThread.cpp line 312:

> 310:   register_task(&remset_task);
> 311:
> 312:   while (!should_terminate()) {

`should_terminate()` is called in three places/functions; I wonder it's possible to gather them together. Conceptually,
we should check termination condition before yielding to the task or to sleep. Manually inlining `run_tasks` to this
loop actually helps readability, since it's quite short itself.

src/hotspot/share/gc/g1/g1ServiceThread.cpp line 318:

> 316:       _vtime_accum = (os::elapsedVTime() - vtime_start);
> 317:     } else {
> 318:       _vtime_accum = 0.0;

Why resetting it to zero? I thought empty else branch is enough here.

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

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