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

[Stefan Johansson]

On Tue, 20 Oct 2020 10:44:20 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

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

Correct, thanks for spotting that. Will remove the _ms post-fix.

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

I think you are correct but this is pre-existing code that I don't intend to change right now. There is a separate
[issue](https://bugs.openjdk.java.net/browse/JDK-8252645) on moving this to the G1RemSetSamplingTask. Back in the day
when the thread only did remembered set sampling it made sense to measure it here, it doesn't anymore, but I wanted to
leave that out of this change.

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

I agree that it's a bit unfortunate and I can see if I can find some way to restructure the code to improve this.
Thinking about it, the recent changes might make it possible to nicely just let each round in the outer loop handle one
task and then check if we should sleep, maybe this is what you meant.

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

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