Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return a Promise<void> instead of a boolean #15

Open
TimvdLippe opened this issue Aug 8, 2019 · 18 comments
Open

Return a Promise<void> instead of a boolean #15

TimvdLippe opened this issue Aug 8, 2019 · 18 comments

Comments

@TimvdLippe
Copy link

Not sure if this has been considered, but reading the proposed API makes me wonder if the API can return a Promise instead that is resolved once input is no longer pending (for the provided InputType):

let taskQueue = [task1, task2, ...];

async function doWork() {
  while (let task = taskQueue.pop()) {
    task.execute();
    await navigator.scheduling.noInputPending(['click', 'keydown', ...]));
  }
}

doWork();

To my understanding, this would solve ~3 problems that I thought of when analyzing the boolean-returning proposal.

It is unclear when input is no longer pending

With isInputPending, if it returns true, it is unclear at what point in time it no longer is pending. In the example defined in the README, a setTimeout(task, 0) is used to wait and check again. However, if I am not mistaken, setTimeout provides no guarantee that input is no longer pending. Similarly, I think it does not provide a guarantee that tasks are further resolved in order when isInputPending was invoked.

Control-flow is broken

Continueing on the previous point, if a setTimeout is invoked, the JavaScript following the original invocation of doWork will continue.
For example:

let taskQueue = [task1, task2, ...];

function doWork() {
  while (let task = taskQueue.pop()) {
    task.execute();
    if (navigator.scheduling.isInputPending(['click', 'keydown', ...])) {
      setTimeout(doWork, 0);
      break;
    }
  }
}

doWork();

performSomeWorkThatActuallyReliesOnTheOutputOfDoWork();

Since doWork breaks out of the while-loop, the performSomeWorkThatActuallyReliesOnTheOutputOfDoWork does not actually have any guarantee that the actions of doWork have fully resolved. This problem is more prevalent if doWork is put in a different file/library where the call-side does not know that doWork is trying to be friendly to the browser by yielding. E.g. reading the following code would be problematic, as there is no way of knowing whether doWork was actually finished:

import {doWork} from './worker.js';

function performSomeWorkThatActuallyReliesOnTheOutputOfDoWork() {
  <...>
}

doWork();
performSomeWorkThatActuallyReliesOnTheOutputOfDoWork();

Granted, it is quite easy to work around these kind of issues, by wrapping the setTimeout in a Promise that you await, for which you make doWork async. However, as every call-side would need to do that, wrapping it in a Promise from the get-go would prevent that boilerplate.

Completion violation

This is where I am not sure if it actually would resolve the issue outlined by @domenic in #12. My reading of a Promise-based API would mean that it would not violate the completion principle, but I might be missing internal details here.

isFramePending || isInputPending -> noBrowserTaskPending

All in all, the isFramePending proposal Discourse would have the same issue. Using them together could become quite clunky:

function doWork() {
  while (let task = taskQueue.pop()) {
    task.execute();
    if (navigator.scheduling.isInputPending(['click', 'keydown', ...])
     || navigator.scheduling.isFramePending()) {
      setTimeout(doWork, 0);
      break;
    }
  }
}

If instead they would be unified to 1 noBrowserTaskPending (naming of course up for discussion), the user could specify it like:

async function doWork() {
  while (let task = taskQueue.pop()) {
    task.execute();
    await navigator.scheduling.noBrowserTaskPending(['click', 'keydown', 'frame', ...]));
  }
}

This feels a lot more natural for both proposals, would circumvent the issues listed above (primarily the we-dont-know-when and control-flow concerns) and would unify the proposals to 1 API. Given that we might want to expose additional pending browser tasks in the future, the user would need to invoke every scheduler function separately which results in a lot of boilerplate.

@n8schloss
Copy link
Collaborator

Using promises here has been brought up a few times in various forums. However, there are concerns since instantiating a promise object takes time as does switching from macro to micro tasks. This API is designed to be called many times in a loop within hot code, so it needs to be able to run really quickly and efficiently. Using a promise here would hurt that premise.

In terms of the promise semantics being easier to understand, someone could absolutely build some kind of promised based API on top the low level API we're proposing here :)

@jakearchibald
Copy link

Aren't we missing the "wait until no input pending" bit, or does requestIdleCallback cover that?

@acomminos
Copy link
Contributor

Aren't we missing the "wait until no input pending" bit, or does requestIdleCallback cover that?

I believe so, yes -- spinning the event loop in requestIdleCallback's start an idle period algorithm should provide this guarantee.

@jakearchibald
Copy link

requestIdleCallback will:

  • Spin the event loop until all the task queues and the microtask queue associated with event_loop are empty.
  • Optionally, wait a further user-agent defined length of time.

Given that it waits for tasks other than input tasks, and waits this extra user-agent defined length of time, it doesn't seem like it compliments isInputPending.

@TimvdLippe
Copy link
Author

Using promises here has been brought up a few times in various forums. However, there are concerns since instantiating a promise object takes time as does switching from macro to micro tasks. This API is designed to be called many times in a loop within hot code, so it needs to be able to run really quickly and efficiently. Using a promise here would hurt that premise.

Do you have data on the performance characteristics on both approaches? Given that I was not the first one to propose Promises, it seems worthwhile to investigate what the actual consequences are. I would personally be surprised if (for a long-running task that already blocks user input and thus requires yielding to the browser) the overhead of a Promise would be significant.

@acomminos
Copy link
Contributor

Sorry for the huge delay here, this totally fell off my radar.

Do you have data on the performance characteristics on both approaches? Given that I was not the first one to propose Promises, it seems worthwhile to investigate what the actual consequences are. I would personally be surprised if (for a long-running task that already blocks user input and thus requires yielding to the browser) the overhead of a Promise would be significant.

For some context, the problem isn't that it's expensive on calls that cause yielding -- we actually need to be concerned that the API is expensive when it doesn't force a yield. This is by far the most common case we can expect.

Consider the case where there's no input pending, and we call a promise-based isInputPending implementation. We force ourselves to bite the performance impact of allocating + chaining a new promise here, irrespective of the fact that there's nothing we had to yield for.

In a release build of Chromium with a naive/idiomatic implementation of this (just allocating the promise, without even chaining), this reduces our ops/s by around 70% [1] using an idiomatic promise resolution strategy. Granted, there's probably room for optimization here, but for such a performance-critical API it seems like we'd want to constrain the UA as little as possible -- especially given that we can achieve the desired semantics using the method described below.

However, if I am not mistaken, setTimeout provides no guarantee that input is no longer pending.

Revisiting this, I think this case is now covered by changes to the spec earlier this year to have isInputPending return true if the UA has enqueued a task to dispatch input. Consider the following:

  • isInputPending will return true if (and only if) a task is enqueued for dispatching input.
  • setTimeout uses the queue a task algorithm, which appends a new task to the end of the task queue.

Therefore, a setTimeout call after isInputPending returns true will have the resulting task dispatch after any pending input tasks are dispatched.


[1] Link to CL here: https://chromium-review.googlesource.com/c/chromium/src/+/2422703

Benchmark results (from the is-input-pending-default-events.html perf test, n=20):

impl avg runs/s median runs/s
isInputPending (boolean) 1693779 1717210
isInputPending (promise, no await) 522345 531041
isInputPending (promise, await) 434317 444458

@jakearchibald
Copy link

  • isInputPending will return true if (and only if) a task is enqueued for dispatching input.
  • setTimeout uses the queue a task algorithm, which appends a new task to the end of the task queue.

Therefore, a setTimeout call after isInputPending returns true will have the resulting task dispatch after any pending input tasks are dispatched.

There isn't a single task queue. Input and setTimeout add items to different queues. Typically browsers will empty input queues ahead of timer queues, but this isn't spec'd or guaranteed.

@yoavweiss
Copy link
Collaborator

Since Promise allocation here is only required in the minority case and is expensive, maybe it's worthwhile to spin it to its own method?

e.g. have a waitOnPendingInput() method that returns a Promise which is resolved when the input task has fired?

That way, developers could do something like:

const DoWorkButYieldWhenNeeded = async () => {
  while (IsWorkToDo()) {
    if (scheduling.isInputPending()) {
      await scheduling.waitOnPendingInput();
    }
    DoShortBurstOfWork();
  }
}

@jakearchibald
Copy link

Should waitOnPendingInput resolve once currently queued inputs have dispatched, or should that also include any additional inputs queued since? Should that be an option?

@acomminos
Copy link
Contributor

There isn't a single task queue. Input and setTimeout add items to different queues. Typically browsers will empty input queues ahead of timer queues, but this isn't spec'd or guaranteed.

Ah, good point.

Since Promise allocation here is only required in the minority case and is expensive, maybe it's worthwhile to spin it to its own method?

Now that I think about it, I believe the goals of such a waitOnPendingInput API are almost entirely satisfied by the proposed scheduler.yield() API. We should be able to satisfy the necessary invariant here for event processing by yielding to work with a priority less than or equal to input-level priority.

Perhaps it makes more sense to pursue the yielding component of this API on that front, which also services the proposed high-level scheduling API?

cc @shaseley

@yoavweiss
Copy link
Collaborator

That does indeed look like a good fit...

@jakearchibald
Copy link

With isInputPending, you get to decide if you're interested in value-update events (like mousemove) or just one-time events like mousedown. sheduler.yield() is a good fit if it offers the same fidelity, but it doesn't look like it does.

The use case here is "I want to perform main thread work while there isn't an input event of a particular category pending". This proposal provides the "you need to stop" part of that, but not "now you can continue" - that requires polling by queuing tasks.

The solution seems incomplete if the "you need to stop" and "now you can continue" APIs are referring to different things, no?

@acomminos
Copy link
Contributor

scheduler.yield() currently accepts the priorities defined in the postTask proposal, one of which is user-blocking (which it's mentioned input events would be categorized as). With such a specificity, we'll yield on at least as much as we checked for via isInputPending (again feel free to correct me if I'm wrong, @shaseley).

I'm not sure we want to permit restricting yielding at an event type granularity, anyway -- that seems like it might introduce semantic issues with task ordering.

@jakearchibald
Copy link

But isInputPending has granularity. You can optionally wait on continual events like mousemove. Why's it ok for the "you need to stop" part to have that granularity, but not the "now you can continue" part?

@acomminos
Copy link
Contributor

acomminos commented Sep 24, 2020

I don't think defining yielding specificity is as important for the use cases of this API. The goal is to reduce input latency in scheduler work loops, which I'm not sure a finer level of yielding granularity would offer us other than a strange contract with developers (though the consistency is appealing).

On the detection front, letting isInputPending ignore continuous events is important for only handling the case of a button press during a large amount of work, but the UA should have the discretion to queue additional work it deems necessary in response to the input as well (e.g. highlights), which is why I think a method to yield only to a single class of input events is tricky.

IMO isInputPending shouldn't drive complete event loop starvation for non-input events, but rather provide a gentle signal to allow the developer to stop doing work if the UA can assure us that input will be processed if we do.

@n8schloss
Copy link
Collaborator

For specifying what you want to wait for when yielding, the conversation here feels increasingly out of scope for this API. isInputPending is about if you should yield or not, scheduler.yield() is more about how to actually yield. It probably makes sense to file an issue on that proposal and to move the conversation there?

@TimvdLippe
Copy link
Author

isInputPending is about if you should yield or not

Right, that part sounds fine to me. However, it is unclear to me how as a webdev I should act on this result. That sounds in scope to me. Whether scheduler.yield() is the appropriate answer or not and how that would look like would be out of scope indeed.

@shaseley
Copy link

A couple quick notes on the status of scheduler.yield:

  • Our current thinking, informed by partner feedback, is that it's more coupled with postTask than we initially thought and are exploring ways to narrow the scope and do something specifically for postTask. We're working through some design right now, and I hope to have something to share soon.
  • Choosing what to yield to is definitely important (this comes up a lot in conversations with React), though we're unclear if that will be in scope for v1 of whatever shape yield takes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants