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

[Albert Mingkun Yang]

On Tue, 20 Oct 2020 12:01:46 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

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

Got it, sure.

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

This looks good; thank you.

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

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