-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
requestIdleCallback: Explicit algorithm for idle callback "deadline" #7166
Conversation
In conjunction with whatwg/html#7166 Instead of defining the idle period deadline in prose, rely on the event model processing in the HTML spec to provide a more precise computation of the current idle period deadline. This is accomplished by passing a `getDeadline` algorithm from the HTML spec to this spec when starting an idle period, and re-computing that deadline between idle callbacks, or when `timeRemaining()` is called. This ensures (more formally) that adding timeouts that expire before the end of the idle period, or `requestAnimationFrame` calls from within the idle period, which currently are specified to fire before the next idle tasks due to event loop priorities, will also be reflected when calling `timeRemaining`. Closes #71
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!! A couple of comments
source
Outdated
<p>Let <var>estimatedCallbackTime</var> be <var>startTime</var> plus <var>timeout</var>, | ||
plus the duration in milliseconds in which the document has not been | ||
<span>fully active</span> since <var>startTime</var>, plus a number representing an | ||
<span>implementation-defined</span> length of time in milliseconds.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a note indicating what that implementation-defined time represents?
source
Outdated
spec="REQUESTIDLECALLBACK"></p> | ||
<p>then for each <code>Window</code> <var>win</var> in the <span>same-loop windows</span> for | ||
this <span>event loop</span>, <span>start an idle period</span> for <var>win</var> with the | ||
following steps to compute the current idle period deadline: <ref spec=REQUESTIDLECALLBACK></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In w3c/requestidlecallback#95 you define passing an algorithm to "start an idle period". It'd be better to make it more explicit here (e.g. by naming the algorithm and passing it).
Deadline should be: - capped at 50ms - capped at 1000/60 (~16ms) when there is a pending rAF callback - capped at the time of the next timeout - be updated when the above conditions change, during the callback itself See whatwg/html#7166
d706afc
to
0f4aab0
Compare
@domenic: Thanks for the review, I revised the PR based on your comments. I think it's more readable now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely on the right track.
0f4aab0
to
490f0f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice upgrade in rigor.
Can you confirm that this bfcache logic is implemented? It's a decent bit of complexity so it seems good to double-check. On the other hand I see the spec already has something similar for setTimeout ("wait until the Document associated with method context has been fully active for a further timeout milliseconds (not necessarily consecutively).") so maybe it just falls out of that infrastructure automatically, in implementations.
source
Outdated
|
||
<ol> | ||
<li><p>Let <var>nextRenderDeadline</var> be this <span>event loop</span>'s | ||
<span>last render opportunity time</span> plus 1000 / 60.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add "(i.e., approximately 16.67ms)" since that could help people understand the intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. After the input from Apple, I'd also want to mention here that the frame-rate can be set by the user-agent and 60FPS is more of a default.
source
Outdated
</li> | ||
|
||
<li><p>For each <var>win</var> of the <span>same-loop windows</span> for | ||
this <span>event loop</span>, <span>start an idle period algorithm</span> for <var>win</var> with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either "start an idle period" or "perform the start an idle period algorithm"
source
Outdated
|
||
<li><p>For each <var>handle</var> in <var>newDocument</var>'s | ||
<span>suspended timer handles</span>, if the <span>map of active timers</span> contains | ||
<var>handle</var>, increase <span>map of active timers</span>[handle] by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<var>
around handle
, and closing </p></li>
source
Outdated
<var>newDocument</var>'s <span>suspension time</span>.</p></li> | ||
|
||
<li><p>For each <var>handle</var> in <var>newDocument</var>'s | ||
<span>suspended timer handles</span>, if the <span>map of active timers</span> contains |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which map of active timers? Maybe newDocument's relevant global object's map of active timers?
source
Outdated
@@ -88382,6 +88385,14 @@ new PaymentRequest(…); // Allowed to use | |||
then:</p> | |||
|
|||
<ol> | |||
<li><p>Set <var>suspendDuration</var> to the <span>current high resolution time</span> minus | |||
<var>newDocument</var>'s <span>suspension time</span>.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent of these steps is to only run on bfcache restore, right?
I think they all work as written, because in non-bfcache cases "suspended timer handles" will be empty. But it's a bit confusing. My suggestion would be to add a redundant guard/assert to make it clear:
- If newDocument's suspended timer handles is not empty:
- Assert: newDocument's suspension time is not 0, i.e. we are restoring from the back/forward cache.
- Set suspendDuration...
- For each handle in...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also for pagevisibility
changes, optionally in a later revision (some browsers do this).
But the assert would work for both cases, sure. There shouldn't be a case where resume
is called without a suspend
.
source
Outdated
object on which the method was invoked, the method does nothing.)</p> | ||
<span data-x="map remove">remove</span> <span>map of active timers</span>[<var>handle</var>] of | ||
the <code>WindowOrWorkerGlobalScope</code> object on which the method was invoked, if any, where | ||
<var>handle</var> is the argument passed to the method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing </p>
4dc7ea8
to
ea2a8da
Compare
In cases where the document goes into suspension in a salvageable state when there active timers, this is implemented (e.g. in Safari). WebKit in general is more bullish than that, also suspending/resuming timers when the page becomes hidden/visibile. The BFCache case is necessary and was there in the spec before this change as you've mentioned - otherwise the suspended document might have all kinds of DOM references to defunct timers. Implementations that don't want to support this can choose to not mark documents with active timers as salvageable, and only store them in BFCache if they don't have such timers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Only one question remaining..
@@ -96323,6 +96434,11 @@ enum <dfn enum>DOMParserSupportedType</dfn> { | |||
<li><p>Let <var>task</var>'s <dfn>timer nesting level</dfn> be <var>nesting | |||
level</var>.</p></li> | |||
|
|||
<li><p>Let <var>startTime</var> be the <span>current high resolution time</span>.</p></li> | |||
|
|||
<li><p><span data-x="map set">Set</span> <span>map of active timers</span>[<var>handle</var>] to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step conflicts with the existing (and now kinda broken) step 3.
I think the correct fix is just to delete step 3. But I wanted you to double-check the interaction with previous handle, which is used for setInterval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double checked and deleted step 3, it's a leftover. Interaction with previous handle
which is used for setInterval
is fine... with every interval the target timestamp would be updated and the map entry would be set to that new timestamp.
The algorithm replaces the algorithm that is currently defined in prose here: https://w3c.github.io/requestidlecallback/#the-idledeadline-interface). It works as such: - Timers can calculate their estimated next callback timestamps, based on their start time and document-inactive time so far. - The deadline for idle tasks for an event loop is the earliest between: - The time until the next timer callback in this event loop is estimated to fire. - The time until the next render, estimated at16ms after the start of the previous task that had a rended opportunity, if there are pending requestAnimationFrame callbacks. - 50ms TODO: the deadline is currently checked between windows, it needs to be re-checked between callbacks in the same window. See w3c/requestidlecallback#71
4a1ca89
to
3af534e
Compare
Deadline should be: - capped at 50ms - capped at 1000/60 (~16ms) when there is a pending rAF callback - capped at the time of the next timeout - be updated when the above conditions change, during the callback itself See whatwg/html#7166
Deadline should be: - capped at 50ms - capped at 1000/60 (~16ms) when there is a pending rAF callback - capped at the time of the next timeout - be updated when the above conditions change, during the callback itself See whatwg/html#7166
* Tests for requestIdleCallback deadline computation Deadline should be: - capped at 50ms - capped at 1000/60 (~16ms) when there is a pending rAF callback - capped at the time of the next timeout - be updated when the above conditions change, during the callback itself See whatwg/html#7166 Co-authored-by: Philip Jägenstedt <philip@foolip.org>
…omputation, a=testonly Automatic update from web-platform-tests Tests for requestIdleCallback deadline computation (#31177) * Tests for requestIdleCallback deadline computation Deadline should be: - capped at 50ms - capped at 1000/60 (~16ms) when there is a pending rAF callback - capped at the time of the next timeout - be updated when the above conditions change, during the callback itself See whatwg/html#7166 Co-authored-by: Philip Jägenstedt <philip@foolip.org> -- wpt-commits: 6ae31b2052754cb7645ba0e4c7c8ec43681d4a36 wpt-pr: 31177
…omputation, a=testonly Automatic update from web-platform-tests Tests for requestIdleCallback deadline computation (#31177) * Tests for requestIdleCallback deadline computation Deadline should be: - capped at 50ms - capped at 1000/60 (~16ms) when there is a pending rAF callback - capped at the time of the next timeout - be updated when the above conditions change, during the callback itself See whatwg/html#7166 Co-authored-by: Philip Jägenstedt <philip@foolip.org> -- wpt-commits: 6ae31b2052754cb7645ba0e4c7c8ec43681d4a36 wpt-pr: 31177
…s#31177) * Tests for requestIdleCallback deadline computation Deadline should be: - capped at 50ms - capped at 1000/60 (~16ms) when there is a pending rAF callback - capped at the time of the next timeout - be updated when the above conditions change, during the callback itself See whatwg/html#7166 Co-authored-by: Philip Jägenstedt <philip@foolip.org>
The algorithm replaces the algorithm that is currently defined in prose
here: https://w3c.github.io/requestidlecallback/#the-idledeadline-interface).
It works as such:
based on their start time and document-inactive time so far.
to fire.
the previous task that had a render opportunity, if there are
pending animation frame callbacks (or if the user agent believes a render is coming).
requestIdleCallback
spec, so that it can always return the up-to-date deadline.See w3c/requestidlecallback#71
(See WHATWG Working Mode: Changes for more details.)
/browsing-the-web.html ( diff )
/infrastructure.html ( diff )
/timers-and-user-prompts.html ( diff )
/webappapis.html ( diff )
/workers.html ( diff )