WICG / scheduling-apis

APIs for scheduling and controlling prioritized tasks.
https://wicg.github.io/scheduling-apis/
Other
908 stars 45 forks source link

Should yield() with explicit priority/signal change the inherited state? #96

Closed shaseley closed 1 month ago

shaseley commented 2 months ago

Currently, scheduler.yield() doesn't update the inherited state, it's only reads it and uses it to schedule continuations. But if an explicit signal/priority are provided, should that be used for the duration of the continuation task so that future continuations (of that continuation) inherit that new state? This was an open question which hasn't been resolved yet.

Example 1:

async function task() {
  await doStuffThatMightYield();
  doStuff();
  await scheduler.yield();
  doMoreStuff();
}

scheduler.postTask(task, {priority: 'background'});

Example 2:

async function task() {
  doStuff();
  // Should this change the context...
  await scheduler.yield({priority: 'background'});
  // ...so that this uses the same signal?
  await doSomethingElseThatMightYield();
}

setTimeout(task);

The reason I chose the current behavior:

  1. I like the simplicity of only setting the context on "task entry points" (scheduler.postTask() and requestIdleCallback() callbacks, otherwise none)
  2. In example (1), I thought it was unexpected that doStuffThatMightYield() might change the context such that the .yield() in task isn't 'background' priority. That seemed like enough of a footgun to avoid.

But in example (2), is it surprising that doSomethingElseThatMightYield() doesn't use background priority? And are there common examples of where this might be important in practice, e.g. framework code calling into non-framework code/observers?

Maybe the existing behavior is a better default, and the alternative is to schedule tasks when you want to start a new context? This would be simpler if we eliminated the signal/priority options, but I think having the options makes the API easier to use outside of .postTask().

shaseley commented 2 months ago

@mmocny I'm curious of your thoughts on this.

mmocny commented 2 months ago

Hmmm.

My initial preference was to expect not the current behaviour, but to instead update the inherited state.

Without this, the behaviour of yield feels inconsistent. The same function which calls yield() only, and which might currently be running at background priority will becomes re-scheduled at an arbitrary priority based on the initial task.

Slightly tweaking your examples:

async function task() {
  await doStuffThatMightYield();
  doStuff();
  await scheduler.yield({priority: 'background'});
  // Could I force this to remain background unless explicitly requested otherwise?
  doSomethingElseThatMightYield();
}

scheduler.postTask(task); // Option 1
scheduler.postTask(task, {priority: 'background'}); // Option 2

If you did want to "force" to remain background, I guess you could schedule with a new postTask and a new priority, but then you lose yield semantics. (Though, I cannot think of a concrete example where that would be important).

So it seems to be that:


EDIT: I think that last suggestion is consistent with your suggestion to remove the signal/priority options from yield api, which I think is cleaner-- yet still provides a way to change priority/signal without resetting task context.

However, after some more thought, I'm not sure if that could be surprising for any other followup code that runs in the same task to update the "global" for the current context...

Also, maybe we don't actually need yield semantics + priority change, as I suggested.

A postTask changing to a higher priority would run before any yield points at the current priority, anyway.

The only possible difference with yield semantics is if there were other tasks at that new priority in the task queue and now you have to go to the back of the line-- but maybe that's expected, and this seems only a problem for decreasing priority anyway-- since the current task was just scheduled at the current priority, any higher priority queue must have been recently empty.

mmocny commented 2 months ago

(Warning, this is playing devil's advocate / musing:)

That isn't an actual suggestion. I think that might be and acceptable choice for task scheduling, but I think it falls apart with other scheduling mechanisms, especially waiting for async apis like fetch, where a use-blocking task should be able to pass priority to its scheduled fetch handler task.

shaseley commented 2 months ago

My initial preference was to expect not the current behaviour, but to instead update the inherited state.

Without this, the behaviour of yield feels inconsistent. The same function which calls yield() only, and which might currently be running at background priority will becomes re-scheduled at an arbitrary priority based on the initial task.

Yeah, I think that makes sense from a consistency perspective: .yield() by default inherits the priority of the current HTML task, or user-visible if the task has no priority (vs. initial task priority).

Thus, any function, if it's continuation were scheduled at an unpredictable and inherited priority, which could be background, could lead to increased latency that might be surprising. This might motivate libraries to more often specify explicit priority with yield, which we don't really want.

  • If not by default, then it would be useful to allow some mechanism to use yield semantics + update to inherited priority

    • Perhaps if there was a way to get a reference to the current Task's TaskController and you could call setPriority first, then just yield()?

EDIT: I think that last suggestion is consistent with your suggestion to remove the signal/priority options from yield api, which I think is cleaner-- yet still provides a way to change priority/signal without resetting task context.

However, after some more thought, I'm not sure if that could be surprising for any other followup code that runs in the same task to update the "global" for the current context...

Yeah, exposing the controller could have other effects, e.g. if the same signal was used to schedule other tasks, which makes it different than just changing the priority of subsequent continuations (a subset of possible tasks having the same signal).


  • Any task, if it were to choose NOT to yield() at some hypothetical potential yield point, its "continuation" would effectively be at the highest priority (aka, instant and blocking)
  • Thus, any task, at any priority even background, when actually choosing to yield(), is strictly better than not, even if its continuation was scheduled at a high priority (even user-blocking), it would still be better than not yielding.
  • Thus, any function, if it's continuation were scheduled at an unpredictable and inherited priority, which could be background, could lead to increased latency that might be surprising. This might motivate libraries to more often specify explicit priority with yield, which we don't really want.
  • This isn't a question about yield() writing to the inherited priority, it seems like a question about yield() even reading inherited priority in the first place.
  • In other words, perhaps priority should only apply for a single scheduling hop, and all follow-up work is either purely blocking or yields with normal priority (which is fairly consistent)

That isn't an actual suggestion. I think that might be and acceptable choice for task scheduling, but I think it falls apart with other scheduling mechanisms, especially waiting for async apis like fetch, where a use-blocking task should be able to pass priority to its scheduled fetch handler task.

I agree that any yielding is better than no yielding (for responsiveness), and the main issue is latency. I like inheritance especially for "background" tasks: when breaking up a low priority task, inheritance is a really good default, assuming the whole task was low priority and not just the first chunk. Of course without inheritance (or explicit priority setting), yieldy "user-blocking" tasks don't have "yield semantics" in presence of other "user-blocking" tasks.


I think I agree that the most consistent thing would be to update the inherited state, but I'm wondering how concerned to be about code calling into async library code using .yield(), which might change the state unexpectedly. Does this lead to a similar problem you mention above, where priority/signal gets passed explicitly more frequently? Or is the most likely thing that the library just calls plain .yield()? (I think it's a feature of inheritance that libraries don't need to be concerned with priority -- it's set by the caller).

The main reason "priority" is an option for yield is for using the API without scheduler.postTask() and wanting to control continuation priority, e.g. input event listeners. But you could just use scheduler.postTask() for that.

It's certainly a simpler model if we exclude it:

[1] But what about abort? This has the same problems as priority, so I guess it would make sense to exclude this also, but I'm not sure I like that...

shaseley commented 2 months ago

Thinking about this a bit more, implementing the context change given an explicit priority/signal is tricky:

async function task() {
  // This binds the current state to the promise continuation object created in the await call...
  await scheduler.yield({priority: 'background'});
  // .. which is restored by the JavaScript engine, here --- during the microtask checkpoint.
  doStuff();
  await scheduler.yield();
}

scheduler.postTask(task);

To overwrite the context just after the first await, so it's associated with the next await, we'd need to do one of:

  1. Change the context in JS (e.g. new API), after the first await but before doStuff()
  2. Overwrite the current context for the duration of the microtask checkpoint, which I think would require adding hooks in ECMAScript
  3. Dynamically change the stored context, or what the stored contents maps to -- but I'm not sure if that's possible
  4. Something else?

My feeling is that this might be more complexity than it's worth, especially without knowing the demand for context change. It might be better to start with inherit-only yield(); we could always add the options back if we find a strong need for it.

mmocny commented 1 month ago

Is this resolved via removing the option to override those values?

shaseley commented 1 month ago

Is this resolved via removing the option to override those values?

Yep, closing --- we can revisit if demand for yield() parameters arises based on usage.