Closed Bug 1353206 Opened 7 years ago Closed 7 years ago

expose requestIdleCallback (or similar) API to non-DOM JS execution contexts

Categories

(Core :: XPConnect, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Performance Impact high
Tracking Status
firefox55 --- fixed

People

(Reporter: dmosedale, Assigned: farre)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

This came up in a Quantum Flow meeting last week.  It may be useful to have a requestIdleCallback-style API in non-DOM contexts so that code that can't easily be moved off of performance-critical threads can at least try to get out of the way.  Thoughts?

(No clue what the right component for this is; feel free to move it whereever).
It seems reasonable to me, perhaps as something that can be imported by Components.utils.importGlobalProperties.

Bugs to add new global properties to that method generally fall into Core::XPConnect or Core::DOM.  Moving this into the former.
Component: Embedding: APIs → XPConnect
Whiteboard: [qf]
Depends on: 1311425
I agree that this would be a good thing. I see two things that we need to think about: 

* the first is the bug I just linked, which is that we need to make sure as there would be more idle runnables we make sure that they won't delay timers

* secondly, Window.requestIdleCallback uses a built in per-window-queue as to not flood the idle queue, and this is something that we might want to keep doing to make idle runnables have similar priority
I think we have some amount of this already exposed via nsIThread's idleDispatch(), right?
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #3)
> I think we have some amount of this already exposed via nsIThread's
> idleDispatch(), right?

It's [noscript] though.
Priority: -- → P2
(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> (In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment
> #3)
> > I think we have some amount of this already exposed via nsIThread's
> > idleDispatch(), right?
> 
> It's [noscript] though.

Yes, realized that, and that is why I change my mind about doing bug 1341645 as a first step. That would indeed be a good thing, because then we'd take care of the second point of comment 2 I think.
Whiteboard: [qf] → [qf:p1]
Can we find an owner for this, please? This is pretty important for the front-end folks to be able to schedule their async work better.  Thanks!
Flags: needinfo?(overholt)
Oh, for some reason I thought we had this done already. Andreas, please hook front-end code up with rIC's awesomeness!
Assignee: nobody → afarre
Flags: needinfo?(overholt)
Perhaps I'm oversimplifying to a great extent, but could we make nsIThread's idleDispatch not [noscript]?
Just exposing something simple like idleDispatch should unblock us, yes. But I think in lots of cases we'll end up combining a timer with it, which seems a bit unfortunate.

More specifically, I think we'll have cases where we'll want to run something while idle, but after waiting at least some amount of time (that will be for example the session store case). If we just add idleDispatch, we'll need to keep the current timer, and call idleDispatch from the timer's callback. That seems to me like it would still add overhead.

We will also have cases where we want to run something while idle, but wait no more than a few seconds, to ensure the runnable gets executed even if something else keeps queuing events. requestIdleCallback supports that case.
Right, we need timers. I made this Bug 1358476 over the weekend. Pretty untried though.
Depends on: 1358476
Hi Andreas, do you expect this to be unblocked soon? It's blocking several photon-performance bugs, and doesn't seem to have progressed in a month (although I see there are recent patches in bug 1358476, so maybe most of the work is happening elsewhere? :-)).
Flags: needinfo?(afarre)
So it turns out that as long as you have a window, this already works in Chrome code. That is, wherever you can do setTimeout(_ => /* something */) you should be able to do requestIdleCallback(_ => /* something */) or requestIdleCallback(_ => /* something */, {timeout: 1000}). 

The work happening in bug 1358476 is for C++ and when you can get hold of a thread, but not a window.
Flags: needinfo?(afarre)
(In reply to Andreas Farre [:farre] from comment #12)
> So it turns out that as long as you have a window, this already works [...]

I did a quick hack to verify this, but I'm no front end dev, so I hope that I'm correct :) 

> The work happening in bug 1358476 is for C++ and when you can get hold of a
> thread, but not a window.

Should be _or_ when you can get hold of a thread, but not a window. That is, both for C++ and non-C++.
(In reply to Andreas Farre [:farre] from comment #13)
> (In reply to Andreas Farre [:farre] from comment #12)
> > So it turns out that as long as you have a window, this already works [...]
> 
> I did a quick hack to verify this, but I'm no front end dev, so I hope that
> I'm correct :) 

The whole point of this bug is that we can't access a window from XPCOM components and JavaScript modules (that's what "non-DOM contexts" means in this bug's summary), so we need a scriptable xpcom methods for these cases. Something like http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/xpcom/threads/nsIThreadManager.idl#92
(In reply to Andreas Farre [:farre] from comment #15)
> Created attachment 8869020 [details] [diff] [review]
> 0001-Bug-1353206-Expose-nsIThread-idleDispatch-to-script..patch
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=96792d464434168b3b0aafdefe3031dd26c62c44

This is the bare basic version of what we want. This does not expose handling of either deadlines or timeouts.
Blocks: 1365970
Florian: for some cases, I wonder if using the hidden DOM window would work well enough?
Which is to say:

Services.appShell.hiddenDOMWindow.requestIdleCallback(...)
Blocks: 1357114
Hey Andreas, did you mean to request review on this patch?
Flags: needinfo?(afarre)
Flags: needinfo?(afarre)
Attachment #8869020 - Flags: review?(nfroyd)
Comment on attachment 8869020 [details] [diff] [review]
0001-Bug-1353206-Expose-nsIThread-idleDispatch-to-script..patch

Review of attachment 8869020 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below changes.

::: xpcom/threads/nsIThread.idl
@@ +139,5 @@
>     * from any thread, and it may be called re-entrantly.
>     *
>     * @param event
> +   *   The (raw) event to dispatch.
> +   *   NOTE that the event will be leaked if it fails to dispatch.

This documentation for `event` should be moved to the scriptable version and the "NOTE" removed.

@@ +142,5 @@
> +   *   The (raw) event to dispatch.
> +   *   NOTE that the event will be leaked if it fails to dispatch.
> +   *
> +   * @throws NS_ERROR_INVALID_ARG
> +   *   Indicates that event is null.

nsIEventTarget::dispatch can fail with NS_ERROR_UNEXPECTED (or similar) if the thread has shutdown, should we document that here as well?

@@ +155,2 @@
>     *   The alreadyAddRefed<> event to dispatch.
>     *   NOTE that the event will be leaked if it fails to dispatch.

This documentation for `event` should be moved to the [noscript] version.

@@ +155,5 @@
>     *   The alreadyAddRefed<> event to dispatch.
>     *   NOTE that the event will be leaked if it fails to dispatch.
>     *
>     * @throws NS_ERROR_INVALID_ARG
>     *   Indicates that event is null.

Ditto on documenting what happens when you dispatch after the thread has shutdown.
Attachment #8869020 - Flags: review?(nfroyd) → review+
Fixed review issues, letting r+ carry over.
Attachment #8869020 - Attachment is obsolete: true
Attachment #8870754 - Flags: review+
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b786cc1d17a
Expose nsIThread::idleDispatch to script. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/9b786cc1d17a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
No longer blocks: 1357114
No longer blocks: 1365970
Blocks: 1373408
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.