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

[Thomas Schatzl]

On Tue, 27 Oct 2020 21:36:32 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 four additional commits since the last revision:
> 
>  - Aditional gtest and assert
>  - Review tschatzl: Explicit reschedule call in task
>  - Review tschatzl: Use elapsed_counter
>  - Review tschatzl: Spelling and such

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/g1/g1ServiceThread.hpp line 73:

> 71:   // The sentinel task is the entry point of this priority queue holding the
> 72:   // service tasks. The queue is ordered by the time the tasks are scheduled
> 73:   // to run and the sentinel task has the time set to DBL_MAX. This guarantees

s/DBL_MAX/max_jlong. Maybe just something like: the sentinel task is put at the end of the queue to simplify list management.

src/hotspot/share/gc/g1/g1ServiceThread.hpp line 46:

> 44:   G1ServiceTask(const char* name);
> 45:   const char* name();
> 46:   void set_service_thread(G1ServiceThread* thread);

maybe just make the `ServiceThread` a friend of `ServiceTask(Queue)` to avoid the setters (`set_service_thread` and `set_time` and probably others)?
In the end you can't use one without the others anyway, i.e. they are already very tightly coupled.
These management methods seem to only clutter the public interface (i.e. you can still use the methods internally)

src/hotspot/share/gc/g1/g1ServiceThread.hpp line 105:

> 103:   // Used both to determine if there are tasks ready to run and
> 104:   // how long to sleep when nothing is ready.
> 105:   int64_t time_to_next_task_ms();

I recommend returning a positive number here because it only raises questions what this means. The implementation of the method also never returns negative values.

src/hotspot/share/gc/g1/g1ServiceThread.hpp line 114:

> 112:   G1ServiceThread();
> 113:   double vtime_accum() { return _vtime_accum; }
> 114:   void register_task(G1ServiceTask* task, jlong delay = 0);

Please add a comment what the delay means, i.e. something like: automatically schedules the task with the given delay.

>From what is written here I am actually a bit confused about the difference between `register_task()` and `schedule_task()`. One seems to be to use by *other* threads, and the other one should only be called by a `ServiceTask` (in context of the `ServiceThread`).

Do we actually need to make a difference between these two and/or do both need to be public? At first glance I would not be sure who is allowed to call which and what the difference actually is.
It also seems that if either is called by the wrong thread bad things happen. Maybe it is possible to add some asserts to warn about most unintentional misuse?

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

> 350:   assert(task != NULL, "not a valid task");
> 351:   assert(task->next() == NULL, "invariant");
> 352:   assert(task->time() != DBL_MAX, "invalid time for task");

s/DBL_MAX/jlong_max ?

src/hotspot/share/gc/g1/g1ServiceThread.hpp line 70:

> 68: };
> 69: 
> 70: class G1ServiceTaskQueue {

Maybe add:
// Time ordered list of tasks to execute.

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

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