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

Interaction with requestIdleCallback spec is very bogus #108

Closed
domenic opened this issue Feb 26, 2016 · 21 comments
Closed

Interaction with requestIdleCallback spec is very bogus #108

domenic opened this issue Feb 26, 2016 · 21 comments

Comments

@domenic
Copy link

domenic commented Feb 26, 2016

@annevk pointed this out over in whatwg/html#708 (comment).

The spec currently says:

Enqueue a task to notify intersection observers in the list of idle request callbacks with a timeout of 100.

There are several issues here:

  • Enqueuing a task in a list of callbacks makes no sense. Callbacks and tasks are fundamentally different things. I think the RIC spec needs to generalize itself to allow a queue of idle tasks, not just callbacks; some of those tasks might be "call a callback" tasks, but others might be "notify intersection observers" tasks. It also needs to define what enqueuing on this queue means.
  • Relatedly, "with a timeout of 100" is meaningless. Just like a queue of callbacks is not a queue of tasks, it is also not a queue of { callback, timeout } pairs. The RIC spec needs to define some operation like "enqueue an idle task with a timeout" and specify how that timeout is processed.

This will presumably require some close work with @rmcilroy (and/or @igrigorik?) to get the RIC spec updated, so pinging them.

@igrigorik
Copy link
Member

I think the RIC spec needs to generalize itself to allow a queue of idle tasks, not just callbacks;

It this dependent on whatwg/html#512? Also, it's not entirely clear to me what an "idle task" means.. a task that's posted by an idle callback? As in, how do we expect these idle tasks to be different from "regular" tasks?

The RIC spec needs to define some operation like "enqueue an idle task with a timeout" and specify how that timeout is processed.

Call requestidleCallback [1] with callback... and options set to...?

[1] https://w3c.github.io/requestidlecallback/#window_extensions

@domenic
Copy link
Author

domenic commented Mar 3, 2016

It this dependent on whatwg/html#512?

No, that seems unrelated.

Also, it's not entirely clear to me what an "idle task" means

It's work that isn't encapsulated by an author-defined JavaScript function, such as notifying intersection observers.

As in, how do we expect these idle tasks to be different from "regular" tasks?

Presumably, they wouldn't be run during the regular event loop. Maybe "task" is a bad word; I meant it in a generic sense, not in the https://html.spec.whatwg.org/#concept-task sense. Although maybe they could be unified.

Call requestidleCallback [1] with callback... and options set to...?

What callback? Fill in the ...s. There is no JavaScript function object (= callback) describing the operation that the spec needs to perform.

@igrigorik
Copy link
Member

Presumably, they wouldn't be run during the regular event loop. Maybe "task" is a bad word; I meant it in a generic sense, not in the https://html.spec.whatwg.org/#concept-task sense. Although maybe they could be unified.

Ah, I think I'm catching up.. So the flow here is: IntersectionObserver runs its logic as part of the render loop (before rAF is called) and it wants to queue a task that should be processed during next idle period.

So, conceptually, we want to extend the processing model to accept + keep a list of "idle period tasks" that are then queued after 7e? Each accepted task would also replicate riC step 6, which runs a timer...

Does that sound about right?

/cc @toddreifsteck

@domenic
Copy link
Author

domenic commented Mar 3, 2016

That sounds about right to me! Although you could maybe simplify the spec by replacing rIC with "queue a idle period task to that calls callback with arguments ..." instead of maintaining a separate runlist and set of idle period tasks.

@rmcilroy
Copy link

rmcilroy commented Mar 8, 2016

So, conceptually, we want to extend the processing model to accept + keep a list of "idle period tasks" that are then queued after 7e? Each accepted task would also replicate riC step 6, which runs a timer...

Sounds reasonable to me.

Although you could maybe simplify the spec by replacing rIC with "queue a idle period task to that calls callback with arguments ..." instead of maintaining a separate runlist and set of idle period tasks.

So would the idea be that we replace list of runnable idle callbacks with a "list of runnable idle tasks" and then step 7c would become "for each entry in doclist, append a task in runlist to call callback with arguments", and then IntersectionObserver could directly post non-callback idle tasks in "list of runnable idle tasks"?

This could work, though we would need to figure out a similar change for the timer event (which is currently handled in the requestIdleCallback algorithm, independent of the idle task processing mode). Perhaps the simplest thing would be for IntersectionObserver to post it's own timeout task independent of the one in rIC, which could cancel the idle task if it gets executed (and vice versa)?

@igrigorik
Copy link
Member

So would the idea be that we replace list of runnable idle callbacks with a "list of runnable idle tasks" and then step 7c would become "for each entry in doclist, append a task in runlist to call callback with arguments", and then IntersectionObserver could directly post non-callback idle tasks in "list of runnable idle tasks"?

Right, I think that makes sense. Also, that would allow us to refactor PerformanceObserver to do the same and start queuing timeline events via rIC.

Perhaps the simplest thing would be for IntersectionObserver to post it's own timeout task independent of the one in rIC, which could cancel the idle task if it gets executed (and vice versa)?

I'd be nice if we could take care of this within rIC, otherwise we'll end up with duplication across a number of specs. As @domenic suggested earlier, we could define a "enqueue an idle task with timeout" within our processing model, and let others call that externally?

@igrigorik
Copy link
Member

@domenic @rmcilroy started drafting an update and in the process realized that we might have a mismatch on our hands...

  • riC invokes the callback with a deadline object, which is used to gauge how much time is left in the idle period - e.g. if period is to short, punt to next idle period, and so on.
  • IntersectionObserver / PerformanceObserver tasks/callbacks, on the other hand, wouldn't get the deadline object (well, unless we change them to explicitly accept it), but would still be candidates for the same idle periods.. which means, they could easily blow the budget on short idle periods without even being aware of it -- that seems bad.

Should IO/PO tasks have special treatment?

  • We could only schedule them during long idle blocks?
  • ... other ideas?

@rmcilroy
Copy link

In my opinion, if APIs are integrating with rIC then the callbacks called via this mechanism should have deadline object arguments, the alternatives seem worse:

  • if we only schedule them during long idle tasks, they will never get run during scrolls / animations / etc, which I guess wouldn't be acceptable to IntersectionObserver (I'm assuming that callers would want to get these callbacks during animations or scrolls as the element becomes visible).
  • if we don't have a deadline object then the callback has no idea how much work it can do before causing jank, limiting the usefulness of being scheduled via rIC in the first place.

Is there any way we could add these deadline argument to the IntersectionObserver / PerformanceObserver specs?

@ojanvafai
Copy link
Collaborator

At the moment, it's not 100% clear we'll be sticking with idle callback timing until we get more experience with this API in the wild. Also, I don't think we could reasonably bake deadlines into this API until requestIdleCallback is more widely supported.

I'm not sure if we can limit them to long idle periods. They have a 100ms deadline though, so I'm not sure how much it matters.

I see that not heaving a deadline isn't ideal, but the alternative is to just post a task, which seems strictly worse to me. Am I thinking about it wrong? The advice to web devs at the moment is to always assume it's running during a short idle period (in fact, the post that @surma is writing explicitly says not to do a lot of work in the callback).

@szager-chromium
Copy link
Collaborator

I think from a developer ergonomics point of view, it would be better to pass a deadline to the callback. But I agree with Ojan that it probably doesn't make sense to tie this not-yet-standard feature to another not-yet-standard feature.

From a strictly practical point of view, this should help address the problem:

function intersectionCallback(changes) {
if (lotsOfWorkToDo(changes)) {
requestIdleCallback(doLotsOfWork.bind(changes));
} else {
doALittleBitOfWork(changes, observer);
}
}

@esprehn
Copy link
Collaborator

esprehn commented Apr 13, 2016

Note that nested requestIdleCallback makes you always miss a frame which sucks, so you're actually delaying yourself potentially another 16ms. I think we should reconsider that design.

@szager-chromium
Copy link
Collaborator

@esprehn Are you sure about that? I'm pretty sure that chromium will run the rIC right away if there's still time left before the next frame.

Anyways, frame timing really shouldn't be an important factor. The notification is already delayed by up to 100ms; what's another 16.6? And if frame timing is important for some reason, then just replace requestIldleCallback with requestAnimationFrame in the example.

@szager-chromium
Copy link
Collaborator

Or, if frame timing is important, just do the work right away.

@igrigorik
Copy link
Member

Ok, let's assume that passing in the deadline object is off the table.

  • Blindly scheduling tasks into a short-idle period (< 16ms) is risky and is likely to cause missed frames.
  • Forcing tasks without deadline into long-idle period (50ms) can delay them for too long.

Can we "compromise" and use a hybrid?

  • Wait up to ~5 frames (80ms) for a long-idle block
  • If 80ms timer has expired, schedule immediately

This would require some additional plumbing in rIC, but I think it strikes a reasonable balance: we don't have to surface the deadline object; we're still able to move these tasks out of the critical path, modulo must-execute-within requirement.

@ojanvafai
Copy link
Collaborator

I think we're over-engineering this. If we had written the spec and implementation to just post a task, we'd be strictly worse off perf-wise and not questioning it. Let's see what happens in the wild with real users of this API before adding complexity. For example, we may well decide that rIC is not acceptable and we should just post a task once we have experience in the wild.

@toddreifsteck
Copy link
Member

The 100 ms deadline wording exists to convince customers to use the API and seems useful to the spec. Generally, I agree that this spec does not need a dependency on requestIdleCallback to achieve its goals and can achieve the same with a queued task and deadline. (If a browser chooses to re-use RIC to implement, that would be up to them.. Walking back to sidelines now)

On Apr 13, 2016, at 7:26 PM, ojan <[email protected]mailto:[email protected]> wrote:

I think we're over-engineering this. If we had written the spec and implementation to just post a task, we'd be strictly worse off perf-wise and not questioning it. Let's see what happens in the wild with real users of this API before adding complexity. For example, we may well decide that rIC is not acceptable and we should just post a task once we have experience in the wild.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//issues/108#issuecomment-209728791

@ojanvafai
Copy link
Collaborator

How do we spec when the queued task fires?

@rmcilroy
Copy link

I agree with Ojan here that we shouldn't try to over-engineering this. If adding deadline objects to the callbacks is off the table, then I agree that we should just run as normal rIC callbacks without a deadline object (not try to wait for a long idle time or otherwise special case the callbacks).

@igrigorik
Copy link
Member

@toddreifsteck the problem is that rIC is the only place that attempts to define the whole concept of idle period (processing). If we unlink the two, then this spec has to make up own definition, which also seems strictly worse.

@ojanvafai @rmcilroy ok, I guess we can start simple and run them as "normal" rIC callbacks without a deadline. With that in mind, it'd be nice if we could provide the #108 (comment) snippet as a recommendation within the IO spec.

@miketaylr
Copy link
Member

I think this was addressed in 2bad9ae. How does https://w3c.github.io/IntersectionObserver/#queue-intersection-observer-task look to you now @domenic? Should we close?

@domenic
Copy link
Author

domenic commented Jun 13, 2022

Yes, looks good!

@domenic domenic closed this as completed Jun 13, 2022
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

8 participants