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

[Stefan Johansson]

On Thu, 19 Nov 2020 08:47:47 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

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

You are correct that doing the extra notification won't hurt, but I don't really see why we should do the notification when we know it is not needed. There are probably a few improvements that we want to do when it comes to how the service thread is registering and scheduling tasks. Since this mechanism is very new I think we will realize how we want this to work more and more.

This is the way I see it right now (after the introduction of the public `G1ServiceThread::schedule_task()`:
* `G1ServiceTask::schedule(delay)` should only be called from a running task. Using it to schedule it from the outside was a bit of a hack. `schedule()` will call `G1ServiceThread::schedule(task, delay)` and nothing more.
* `G1ServiceThread::schedule_task(task, delay)` should be used to schedule a task from the outside. It will under the hood call `G1ServiceThread::schedule(task, delay)` and `G1ServiceThread::notify()` to handle the case where the task end up being the first task in the queue.

To me separating the use cases is good, but you might not agree.  We could add an assert to `G1ServiceTask::schedule(delay)` to ensure that it is only called when running on the service thread, that way we would catch wrong usage of the API quickly.

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

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