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

[Stefan Johansson]

On Fri, 30 Oct 2020 11:26:31 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> 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
>
> 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 ?

Of course, fixed.

> 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.

The reason for using `int64_t` is that `Monitor::wait()` takes an `int64_t`. Albert filed an issue to change that: [JDK-8255119](https://bugs.openjdk.java.net/browse/JDK-8255119), but until that is fixed I prefer using the same type here.

> 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?

I agree these are very similar and as you say it's not good that both are public, I changed this by making `G1ServiceTask` a friend of `G1ServiceThread`, so that `schedule_task()` can be private. Also added short comments to help when only reading the interface.

> src/hotspot/share/gc/g1/g1ServiceThread.hpp line 70:
> 
>> 68: };
>> 69: 
>> 70: class G1ServiceTaskQueue {
> 
> Maybe add:
> // Time ordered list of tasks to execute.

I find this covered by the comment just below.

> 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)

Made both `G1ServiceThread` and `G1ServiceTaskQueue` friends with `G1ServiceTask`, so now all setters are either private or protected. The ones that are protected are used by either tests and/or the sentinel task. These could also be made private and list all tasks needing them as friends, but I went with this solution. What do you think?

> 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.

Fixed.

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

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