Closed
Bug 1353206
Opened 8 years ago
Closed 8 years ago
expose requestIdleCallback (or similar) API to non-DOM JS execution contexts
Categories
(Core :: XPConnect, enhancement, P2)
Core
XPConnect
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: dmosedale, Assigned: farre)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
farre
:
review+
|
Details | Diff | Splinter Review |
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).
Comment 1•8 years ago
|
||
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
Updated•8 years ago
|
Whiteboard: [qf]
Assignee | ||
Comment 2•8 years 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
Comment 3•8 years ago
|
||
I think we have some amount of this already exposed via nsIThread's idleDispatch(), right?
Comment 4•8 years ago
|
||
(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.
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 5•8 years 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.
Updated•8 years ago
|
Whiteboard: [qf] → [qf:p1]
Comment 6•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
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)
Comment 8•8 years ago
|
||
Perhaps I'm oversimplifying to a great extent, but could we make nsIThread's idleDispatch not [noscript]?
Comment 9•8 years ago
|
||
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•8 years ago
|
||
Right, we need timers. I made this Bug 1358476 over the weekend. Pretty untried though.
Depends on: 1358476
Comment 11•8 years ago
|
||
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•8 years 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•8 years 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++.
Comment 14•8 years ago
|
||
(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•8 years ago
|
||
Assignee | ||
Comment 16•8 years 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.
Reporter | ||
Comment 17•8 years ago
|
||
Florian: for some cases, I wonder if using the hidden DOM window would work well enough?
Reporter | ||
Comment 18•8 years ago
|
||
Which is to say:
Services.appShell.hiddenDOMWindow.requestIdleCallback(...)
Comment 19•8 years ago
|
||
Hey Andreas, did you mean to request review on this patch?
Flags: needinfo?(afarre)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(afarre)
Attachment #8869020 -
Flags: review?(nfroyd)
Comment 20•8 years ago
|
||
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•8 years ago
|
||
Fixed review issues, letting r+ carry over.
Attachment #8869020 -
Attachment is obsolete: true
Attachment #8870754 -
Flags: review+
Comment 22•8 years ago
|
||
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b786cc1d17a
Expose nsIThread::idleDispatch to script. r=froydnj
Comment 23•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•