w3c / requestidlecallback

Cooperative Scheduling of Background Tasks
https://w3c.github.io/requestidlecallback/
Other
50 stars 19 forks source link

Change start idle period algorithm to avoid monkey patching event loop. #60

Closed rmcilroy closed 7 years ago

rmcilroy commented 7 years ago

Addresses #57

rmcilroy commented 7 years ago

@annevk, @igrigorik, @toddreifsteck please take a look and let me know what you think.

igrigorik commented 7 years ago

Preview @ https://cdn.rawgit.com/rmcilroy/requestidlecallback/event_loop/index.html

Ross, don't think it's due to this PR, but I see a couple of broken links in the preview:

annevk commented 7 years ago

If there are no tasks on the event loop, you won't necessarily have an animation frame first.

rmcilroy commented 7 years ago

re https://github.com/w3c/requestidlecallback/pull/60#issuecomment-318992607, I'm not sure why this would be an issue. If there are no other tasks / animation frames then we should keep cycling idle periods every 50ms while there are still idle callbacks. Once new tasks / animation frames are posted we should drop back to enabling idle periods in the time between the end of rendering an animation frame and the expected time that the next frame will start running task. Is there something I'm missing?

annevk commented 7 years ago

I thought the requirement was that it happened after the animation frame.

rmcilroy commented 7 years ago

The requirement is that if there is a pending animation frame the idle period must happen after that frame is rendered, otherwise if no frames are being drawn the the idle periods can start any time there are no other tasks in the event loop.

It's difficult to express this in relation to the HTML event loop spec though since there isn't really a concept of "pending animation frame", instead the event loop spec is written such that each spin of the even loop the user agent renders updates for all documents, except those documents that the "user agent believes would not benefit from having its rendering updated at this time". From the attached note it is clear this is intended to enable the browser to only have to render once per frame, but it's not exactly how it's speced.

annevk commented 7 years ago

Hmm, I wonder if we should change that around. Ugh.

I guess this is probably okay then, though I'm a little wary of this depending on a bunch of specific things without reference back from HTML. Seems easy for this to go out of sync.

igrigorik commented 7 years ago

@rmcilroy any thoughts on my review comments above? If those are no-ops, it sounds like we can merge this.

rmcilroy commented 7 years ago

Thanks for the comments @annevk. I'll see if I can include some more specific references back to HTML where it makes sense to reduce the scope for this going out of sync.

@igrigorik yeah I think some of your comments definitely make sense, I just wanted to ensure the overall algorithm was workable before addressing them, I'll take a look at them now.

rmcilroy commented 7 years ago

Ross, don't think it's due to this PR, but I see a couple of broken links in the preview:

Fixed.

Comments addressed, PTAL thanks.

igrigorik commented 7 years ago

Looks good! I see one broken reference, however:

image

rmcilroy commented 7 years ago

Dang, thought I'd fixed that link. Should be fixed now, PTAL thanks.

igrigorik commented 7 years ago

Merging.

@annevk @toddreifsteck if you have any other feedback, please ping here or open a new issue.