RFR: 8256938: Improve remembered set sampling task scheduling

[Stefan Johansson]

On Wed, 25 Nov 2020 13:56:45 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Please review this change to the remembered set sampling task scheduling.
>> 
>> The sampling tasks joins the suspendible thread set (STS) and if it needs to yield during the sampling it aborts and reschedules. This is the expected behavior and will lead to the sampling happening roughly G1ConcRefinementServiceIntervalMillis after the pause ended. But in the case where the task is started during a GC (can happen since the whole service thread isn't joined to the STS) the sampling will start right after the pause has finished. This enhancement changes this by checking when the last pause occurred and reschedules the task to take place G1ConcRefinementServiceIntervalMillis after the GC ended.
>> 
>> **Testing**
>> Tier 1-3 and manual verification looking at logs.
>
> src/hotspot/share/gc/g1/g1RemSet.cpp line 593:
> 
>> 591:       schedule(reschedule_delay_ms());
>> 592:       return;
>> 593:     }
> 
> I would prefer not having `should_reschedule` and reuse the return value of `reschedule_delay_ms`, but up to you.
> 
> delay_ms = reschedule_delay_ms();
> // only reschedule when ...
> if (delay_ms > 1) {
>   schedule(delay_ms);
>   return;
> }
> Second point, why 1 in `reschedule_delay_ms() > 1`, but not 0?

I tried to cover the why in the comment for `should_reschedule()`:
  // To avoid extensive rescheduling if the task is executed a bit early. The task is
  // only rescheduled if the expected time is more than 1ms away.
  bool should_reschedule() {
    return reschedule_delay_ms() > 1;
  }
But maybe this comment needs to be clearer. The problem is that when calculating the `reschedule_delay_ms()` the `since_last_gc.milliseconds()` call might round down to 299ms (when the interval is 300ms), even if we are just a microsecond away. And since there are different timestamps used to decide if the task is ready to run, we can end up in this situation.

Having this comment is one of the reasons I used two functions even if it meant calculating the delay two times. Makes sense? Would you like more info in the comment?

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

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