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

[Thomas Schatzl]

On Thu, 19 Nov 2020 12:02:27 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

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

I would be fine with the assert to prevent misuse.

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

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