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

Missing definition for "idle time" #70

Closed
siusin opened this issue Nov 2, 2017 · 13 comments

Comments

@siusin
Copy link
Contributor

commented Nov 2, 2017

pass on an AC Review comment:

This spec. relies on the concept of "idle time" which might be intuitively understood, but actually isn't defined. Since it's so fundamental to the spec., a definition should probably be made.

... from @dwsinger

@siusin siusin added the enhancement label Nov 2, 2017
@igrigorik

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

@rmcilroy @domenic any thoughts on this one?

We provide a non-normative explainer in https://w3c.github.io/requestidlecallback/#idle-periods. Is there a more formal definition that we could draft here?

@domenic

This comment has been minimized.

Copy link

commented Nov 7, 2017

Could this be defined in terms of https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model ? Something like:

  • There is no task to select in step 1
  • Perform a microtask checkpoint in step 6 finds an empty microtask queue, no rejected promises to notify about, and no IDB transactions to cleanup
  • The UA decided to skip "update the rendering" for all documents controlled by this event loop in step 7
  • Step 8 is not true (otherwise we'd be in the middle of worker shutdown)

We'd need to do some surgery to make this clean and not refer to steps by number, but that would be my first draft.

@toddreifsteck toddreifsteck added this to the Level 1 milestone Nov 7, 2017
@toddreifsteck toddreifsteck added the bug label Nov 7, 2017
@toddreifsteck

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

TPAC 2017:
@rniwa of Apple. Yes, this helps define when Idle period's start may begin if the HTML5 spec is updated in this way. It seems there should be an explicit entry Event Loop after step 7 that this spec should hook.

@rmcilroy

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2017

Yes, I like @domenic's definition above, that sound about right. It would definitely be simpler if we could add an explicit entry into the HTML5 Event Loop after step 7 to hook into. Would someone be willing to add this hook?

@domenic

This comment has been minimized.

Copy link

commented Nov 10, 2017

I can try to work on it, but if someone else has more time, that'd be ideal. I think the trickiest thing is writing the prose to keep track of all the things before and after step 7, so that the new step can confidently say "I am idle".

@igrigorik

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

@domenic any progress on your front? Got a ping from @plehegar on this one. Philippe, you said this is blocking the HTML spec?

@domenic

This comment has been minimized.

Copy link

commented Mar 15, 2018

I'll put up a patch today. However, I'm unsure how exactly to integrate. Right now the idling seems a bit implicit in "start an idle period": requestIdleCallback queues a task, that waits for the event loop to become idle (by spinning it until it's empty), and then queues another task to invoke callbacks.

Probably the simplest integration would be if, when the HTML spec noticed it was idle, it called a single algorithm from this spec. Then you wouldn't do any spinning here. But I don't know what a good candidate for that algorithm would be now. Maybe something like "start an idle period" step 5 onward? Although I think it's weird that step 13 queues a task; we're already on the event loop, so that shouldn't be necessarily, unless you want to add opportunities for the UA to run other tasks that might have accumulated during step 5.

domenic added a commit to whatwg/html that referenced this issue Mar 15, 2018
This only slightly changes the processing model of requestIdleCallback,
by explicitly ensuring that notifying about promise rejections or
cleaning up IndexedDB transactions counts as being non-idle.

Mostly, it makes it clearer what exactly is idle time (per the request
at w3c/requestidlecallback#70), and it calls
directly out to the "start an idle period" algorithm, instead of that
algorithm needing to spin the event loop.
domenic added a commit to domenic/IndexedDB that referenced this issue Mar 15, 2018
It tells you whether any cleanup work was performed or not. This helps with w3c/requestidlecallback#70 and whatwg/html#3570.
@igrigorik

This comment has been minimized.

Copy link
Member

commented Mar 17, 2018

Paging @rmcilroy @plehegar for feedback :)

@rmcilroy

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2018

Thanks for looking into this @domenic! I think integrating into step 5 onwards would be the best bet. The main reason to queue another task in step 13 is for the following:

  • Each idle callback executed by "invoke idle callback algorithm" spins the event loop letting other higher priority tasks have a chance to execute instead of idle tasks
  • We only want to run tasks posted before the current idle period in that idle period (hence appending pending_list to run_list in step 11), to ensure that a task that if reposts itself waiting for a longer deadline (e.g., [1]) won't be re-executed until a new idle period with a (potentially larger) deadline.

Happy to refactor if you see cleaner ways of meeting these goals though.

[1] Taken from https://w3c.github.io/requestidlecallback/#idle-periods

function doWork(deadline) {
  if (deadline.timeRemaining() <= 5) {
    // This will take more than 5ms so wait until we
    // get called back with a long enough deadline.
    requestIdleCallback(doWork);
    return;
  }
  // do work...
}
inexorabletash added a commit to w3c/IndexedDB that referenced this issue May 3, 2018
It tells you whether any cleanup work was performed or not. This helps with w3c/requestidlecallback#70 and whatwg/html#3570.
@yoavweiss

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2018

@rmcilroy @domenic - Is there something delaying progress here?

@domenic

This comment has been minimized.

Copy link

commented Oct 10, 2018

Not that I'm aware of. If someone were able to drive this to completion that would be great.

rmcilroy added a commit to rmcilroy/requestidlecallback that referenced this issue Oct 18, 2018
Makes the "start an idle period" algorithm callable by the HTML event loop
processing model, instead of having this spec spin the event loop.

Addresses w3c#70
rmcilroy added a commit to rmcilroy/requestidlecallback that referenced this issue Oct 18, 2018
Makes the "start an idle period" algorithm callable by the HTML event loop
processing model, instead of having this spec spin the event loop.

Addresses w3c#70
rmcilroy added a commit to rmcilroy/html that referenced this issue Oct 18, 2018
This adds a hook to the event loop's processing model to define when the start
of an idle period should begin. This deals with the issue in
w3c/requestidlecallback#70 and calls directly into the "start an idle period"
algorithm, instead of that algorithm having to having to spin the event loop.

This change depends on the PR in w3c/requestidlecallback#75.
rmcilroy added a commit to rmcilroy/html that referenced this issue Oct 18, 2018
This adds a hook to the event loop's processing model to define when the start
of an idle period should begin. This deals with the issue in
w3c/requestidlecallback#70 and calls directly into the "start an idle period"
algorithm, instead of that algorithm having to having to spin the event loop.

This change depends on the PR in w3c/requestidlecallback#75.
@rmcilroy

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

I've uploaded #75 and whatwg/html#4104 which I think should be sufficient to hook rIC into the HTML spec. @domenic I've gone with a slightly different approach in the event model processing which I think is simpler - in essence instead of working out if we did an idle spin of the loop, instead work out if the next spin of the loop looks to be idle. PTAL, thanks.

domenic added a commit to whatwg/html that referenced this issue Oct 25, 2018
This adds a hook to the event loop's processing model to define when the
start of an idle period should begin. This deals with the issue in
w3c/requestidlecallback#70 and calls directly into the "start an idle
period" algorithm, instead of that algorithm having to having to spin
the event loop.

Follows w3c/requestidlecallback#75. Closes #3570 by superceding it; this
simpler version avoids needing to track notifying above rejected
promises or cleaning up IDB transactions.
@rmcilroy

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

Should now be addressed, thanks all.

@rmcilroy rmcilroy closed this Oct 25, 2018
mustaqahmed added a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
This adds a hook to the event loop's processing model to define when the
start of an idle period should begin. This deals with the issue in
w3c/requestidlecallback#70 and calls directly into the "start an idle
period" algorithm, instead of that algorithm having to having to spin
the event loop.

Follows w3c/requestidlecallback#75. Closes whatwg#3570 by superceding it; this
simpler version avoids needing to track notifying above rejected
promises or cleaning up IDB transactions.
mustaqahmed added a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
This adds a hook to the event loop's processing model to define when the
start of an idle period should begin. This deals with the issue in
w3c/requestidlecallback#70 and calls directly into the "start an idle
period" algorithm, instead of that algorithm having to having to spin
the event loop.

Follows w3c/requestidlecallback#75. Closes whatwg#3570 by superceding it; this
simpler version avoids needing to track notifying above rejected
promises or cleaning up IDB transactions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.