RFR: 8236926: Concurrently uncommit memory in G1 [v7]

[Thomas Schatzl]

On Wed, 18 Nov 2020 14:32:02 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1UncommitRegionTask.cpp line 128:
>> 
>>> 126:     // No delay, reason to reschedule rather then to loop is to allow
>>> 127:     // other tasks to run without waiting for a full uncommit cycle.
>>> 128:     schedule(0);
>> 
>> Why not notify like in `run()`? Maybe refactor the code a bit to allow calling the same method to schedule the task and notify the service thread for immediate processing here.
>
> Because here we know that the service thread is running and if we schedule with a 0 delay, there is no risk of it going to sleep before we run again. Another task might be up next, but this task will eventually run before the service thread can go to sleep.
> 
> There is room for improvement on how we schedule tasks from another thread. I changed `enqueue` to call a new public function on the `G1ServiceThread` called `schedule_task`, which calls `schedule(task, delay)` and then `notify()`. The function `schedule(task, delay)` is what previously was named `schedule_task`. This is what will be called when someone does `task->schedule(delay)` as well, so it is a bit more unified.

My question is mainly, why not notify the task to wake up when scheduling a new task in all cases. I understand the reason for the zero delay.

Not seeing the problem of doing so:

* tasks that need to run (or are overdue) are automatically run before this task as they have a time-to-run < current time, and so this task is scheduled after

* The extra notification for `schedule_task()` does not seem to hurt, at most it wakes up the service thread to do the next scheduled task (which afaiu other tasks if they are already due).

I.e. the "optimization" here to not notify the service thread seems to be superfluous. Or maybe the notification could be suppressed automatically if `schedule_task()` is called in `execute` (`G1ServiceThread` can check fairly easily if it is currently running a task)

I am concerned about users of the API to needlessly have to decide whether they should call `schedule()` or `schedule_task()` as they have different effect.

Maybe `schedule()` could just call `schedule_task()`.

(That might be a pre-existing issue of using `schedule` vs. `schedule_task()`, so feel free to say it's out of scope. :) )

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

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