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

RESOLVED FIXED in Firefox 55

Status

()

Core
XPConnect
P2
normal
RESOLVED FIXED
3 months ago
10 days ago

People

(Reporter: dmose, Assigned: farre)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 months ago
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
Blocks: 1353586
Whiteboard: [qf]
(Assignee)

Updated

3 months ago
Depends on: 1311425
(Assignee)

Comment 2

3 months ago
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
(Assignee)

Comment 5

3 months ago
(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.
(Assignee)

Comment 10

2 months ago
Right, we need timers. I made this Bug 1358476 over the weekend. Pretty untried though.
Depends on: 1358476
Blocks: 1361996
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)
(Assignee)

Comment 12

a month ago
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)
(Assignee)

Comment 13

a month ago
(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
(Assignee)

Comment 15

a month ago
Created attachment 8869020 [details] [diff] [review]
0001-Bug-1353206-Expose-nsIThread-idleDispatch-to-script..patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96792d464434168b3b0aafdefe3031dd26c62c44
(Assignee)

Comment 16

a month ago
(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
(Reporter)

Comment 17

a month ago
Florian: for some cases, I wonder if using the hidden DOM window would work well enough?
(Reporter)

Comment 18

a month ago
Which is to say:

Services.appShell.hiddenDOMWindow.requestIdleCallback(...)

Updated

a month ago
Blocks: 1357114
Hey Andreas, did you mean to request review on this patch?
Flags: needinfo?(afarre)
(Assignee)

Updated

a month ago
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+
(Assignee)

Comment 21

a month ago
Created attachment 8870754 [details] [diff] [review]
0001-Bug-1353206-Expose-nsIThread-idleDispatch-to-script..patch

Fixed review issues, letting r+ carry over.
Attachment #8869020 - Attachment is obsolete: true
Attachment #8870754 - Flags: review+

Comment 22

a month ago
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b786cc1d17a
Expose nsIThread::idleDispatch to script. r=froydnj

Comment 23

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9b786cc1d17a
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a month ago
Blocks: 1368072
No longer blocks: 1357114
No longer blocks: 1365970
Blocks: 1373408
You need to log in before you can comment on or make changes to this bug.