Expose a scriptable equivalent of NS_DispatchToMainThread

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf-])

Attachments

(3 attachments, 4 obsolete attachments)

We have a lot of setTimeout calls with a 0ms time in our front-end code. I think most of this was code written before we started using executeSoon (ie. Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL)).

Is the 0ms timer adding overhead here that we should remove?
Flags: needinfo?(ehsan)
Yes, using executeSoon() would be more efficient.
Flags: needinfo?(ehsan)
Would the module "Firefox:General" be a better fit here?
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> Would the module "Firefox:General" be a better fit here?

For handling the actual replacement in the code, yes.

But I wonder if we need some new native function to be exposed to avoid the overhead of a cross compartement wrapper that we would have if we put an executeSoon implementation in a JS module.
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> > Would the module "Firefox:General" be a better fit here?
> 
> For handling the actual replacement in the code, yes.
> 
> But I wonder if we need some new native function to be exposed to avoid the
> overhead of a cross compartement wrapper that we would have if we put an
> executeSoon implementation in a JS module.

Maybe Andreas has an opinion?
Flags: needinfo?(afarre)
Priority: -- → P2
(In reply to Andrew Overholt [:overholt] from comment #4)
> (In reply to Florian Quèze [:florian] [:flo] from comment #3)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> > > Would the module "Firefox:General" be a better fit here?
> > 
> > For handling the actual replacement in the code, yes.
> > 
> > But I wonder if we need some new native function to be exposed to avoid the
> > overhead of a cross compartement wrapper that we would have if we put an
> > executeSoon implementation in a JS module.
> 
> Maybe Andreas has an opinion?

Depends on what I might have an opinion on :)

If you're thinking idle callbacks, then this isn't really the use case where we'd want to use them. Idle callbacks would be more of a maybeExecuteSoonIfEver(). Not saying that that might be useful though.

About executeSoon, I'm not sure of the usefulness of adding it. I would assume that an implementation of executeSoon would be just a wrapper around nsIThread.dispatch(..., Ci.nsIThread.DISPATCH_NORMAL). Is that really that important just to get the "will execute soon" menmonic? If it is, I guess it's just a matter of adding executeSoon to nsIThread or nsIEventTarget which is pretty simple.
Flags: needinfo?(afarre)
(In reply to Andreas Farre [:farre] from comment #5)

> About executeSoon, I'm not sure of the usefulness of adding it. I would
> assume that an implementation of executeSoon would be just a wrapper around
> nsIThread.dispatch(..., Ci.nsIThread.DISPATCH_NORMAL). Is that really that
> important just to get the "will execute soon" menmonic? If it is, I guess
> it's just a matter of adding executeSoon to nsIThread or nsIEventTarget
> which is pretty simple.

What I was saying is that:
Services.tm.currentThread.dispatch(runnable, Ci.nsIEventTarget.DISPATCH_NORMAL);
is too long, and will cause developers to write a wrapper around it.

Now I just noticed that the value of DISPATCH_NORMAL is 0, which means we could just make that 'flags' parameter optional on the interface and reduce this to:
Services.tm.currentThread.dispatch(runnable)

I think that's short enough that we can use it without wrapper.
The interface change proposed in comment 6.
Attachment #8857461 - Flags: review?(nfroyd)
Assignee: nobody → florian
Status: NEW → ASSIGNED
One thing to be aware of is that in a Quantum DOM world where windows/tabs/document groups get their own event queues, setTimeouts will be ordered with respect to other events happening in that window/tabs/document group whereas dispatching to the main thread will be completely unordered with respect to those other events.  If the handler requires certain things to have happened in the window/tabs/document group, you will see things fail intermittently (or even crash?) when dispatching to the main thread.  So you cannot not convert setTimeout to executeSoon blindly, even though it will work in the present day.

Converting now may make those test failures somebody else's problem, though. :)

I would be ok with adding:

  Services.tm.dispatchToMainThread(runnable)

or some variant thereof, which is likely to be slightly more efficient than:

  Services.tm.currentThread.dispatch(runnable)

That strikes me as slightly cleaner than relying on conversion of null/undefined optional values from xpconnect to zero for the flags, especially since DISPATCH_NORMAL could change its value...
Comment on attachment 8857461 [details] [diff] [review]
make nsIEventTarget.dispatch's flag parameter optional

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

I think I prefer comment 8's approach here.  Please let me know if you feel differently, though!
Attachment #8857461 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #8)
> One thing to be aware of is that in a Quantum DOM world where
> windows/tabs/document groups get their own event queues, setTimeouts will be
> ordered with respect to other events happening in that window/tabs/document
> group whereas dispatching to the main thread will be completely unordered
> with respect to those other events.

Is this only in content processes, or will the per-window event queues also happen in the chrome process?

If the latter, does that mean we should keep something attached to the window, eg. implement window.executeSoon, so that for example when session restore reopens 10 windows at once, we finish initializing the window in the foreground first?

> I would be ok with adding:
> 
>   Services.tm.dispatchToMainThread(runnable)
> 
> or some variant thereof, which is likely to be slightly more efficient than:
> 
>   Services.tm.currentThread.dispatch(runnable)

Services.tm.dispatchToMainThread is fewer characters than Services.tm.currentThread.dispatch so I would be happy about it :).
Quantum DOM is for child processes only.
(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> (In reply to Nathan Froyd [:froydnj] from comment #8)
> > One thing to be aware of is that in a Quantum DOM world where
> > windows/tabs/document groups get their own event queues, setTimeouts will be
> > ordered with respect to other events happening in that window/tabs/document
> > group whereas dispatching to the main thread will be completely unordered
> > with respect to those other events.

This also isn't really true since a task dispatched using nsIThread APIs will by default belong to the same tab/doc group as the one it was originated from, so replacing a setTimeout(0) with disptch() even in the content process like this shouldn't change the ordering of events in any observable way.
Marking qf- since this patch in itself doesn't improve performance.  ;-)
Whiteboard: [qf] → [qf-]
Posted patch dispatchToMainThread WIP (obsolete) — Splinter Review
Here is a WIP patch for the dispatchToMainThread. I'm not familiar at all with this code so I'm only requesting feedback, with a few questions:

- Any idea for a shorter method name?
- Should this be only callable from scripts? Or do we need a second already_addref'ed version for calls from C++?
- Should this be restricted to callers already on the main thread? Can we avoid adding lock overhead in this code path?
Attachment #8858011 - Flags: feedback?(nfroyd)
Comment on attachment 8858011 [details] [diff] [review]
dispatchToMainThread WIP

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

This looks good, thank you.

Answers to questions, in order:

- I don't have great ideas for a shorter method name.  "mainThreadDispatch" is possible, but not parallel to other, similar constructs we have, so I'd like to avoid that.
- There's no way to make it *only* callable from script
- Calls from C++ will just use NS_DispatchToMainThread, and shouldn't be using this method (we may want to note this with a comment in the IDL or implementation or both).
- We could restrict it to callers on the main thread if we wanted, but as calls from C++ shouldn't be using this, per above, and calls from JS should be on the main thread anyway...  MOZ_ASSERT(NS_IsMainThread()) is probably a good idea, though.
- What's the source of extra locking overhead that you are concerned about?
Attachment #8858011 - Flags: feedback?(nfroyd) → feedback+
Attachment #8858075 - Flags: review?(nfroyd)
Comment on attachment 8858075 [details] [diff] [review]
Add dispatchToMainThread, v2

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

Thanks!

::: xpcom/threads/nsIThreadManager.idl
@@ +83,5 @@
> +
> +  /**
> +   * This queues a runnable to the main thread. It's a shortcut for JS callers
> +   * to be used instead of
> +   *   .mainThread.dispatch(runnable, Ci.nsIEventTarget.DISPATCH_NORMAL);

Maybe mention:

  .currentThread.dispatch(...)

as well, since currentThread also seems to be in wide use in JS files, even though it is just mainThread by another name.
Attachment #8858075 - Flags: review?(nfroyd) → review+
Attachment #8858011 - Attachment is obsolete: true
Attachment #8857461 - Attachment is obsolete: true
Posted file xpcshell script (obsolete) —
Comment on attachment 8858093 [details] [diff] [review]
script-generated cleanup patch

Nathan said he can review this.
Attachment #8858093 - Flags: review?(nfroyd)
Rephrasing the bug summary, as the setTimeout replacement will likely happen in another bug at this point.
Summary: Consider replacing setTimeout(0) with executeSoon → Expose a scriptable equivalent of NS_DispatchToMainThread
Comment on attachment 8858093 [details] [diff] [review]
script-generated cleanup patch

Removing the review request for now, the script butchered the cases where the runnable was an arrow function.
Attachment #8858093 - Flags: review?(nfroyd)
Attachment #8858123 - Flags: review?(nfroyd)
Attachment #8858093 - Attachment is obsolete: true
Posted file xpcshell script v2
The location information of the first argument wasn't reliable, this now uses the callee's location instead.
Attachment #8858094 - Attachment is obsolete: true
Comment on attachment 8858123 [details] [diff] [review]
script-generated patch, v2

Flagging Jared too. Whoever gets to rs'ing this scripted change first can feel free to remove the other review request.

The try run in comment 26 is green.

I don't think this is worth adding an eslint rule for, as the existing pattern was obscure enough that I suspect most developers were copy/pasting it rather than typing it, so they'll likely just copy/paste the new pattern.
Attachment #8858123 - Flags: review?(jaws)
Comment on attachment 8858123 [details] [diff] [review]
script-generated patch, v2

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

Thanks!

::: devtools/client/shared/DOMHelpers.jsm
@@ -151,5 @@
>          docShell.chromeEventHandler.removeEventListener("DOMContentLoaded", onReady);
>          // If in `callback` the URL of the window is changed and a listener to DOMContentLoaded
>          // is attached, the event we just received will be also be caught by the new listener.
>          // We want to avoid that so we execute the callback in the next queue.
> -        Services.tm.mainThread.dispatch(callback, 0);

So nice to get rid of places where people don't use the named constant.
Attachment #8858123 - Flags: review?(nfroyd)
Attachment #8858123 - Flags: review?(jaws)
Attachment #8858123 - Flags: review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/781aa8ce66f6
provide a scriptable equivalent of NS_DispatchToMainThread, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/18d45aa984d6
script-generated patch to replace .{currentThread,mainThread}.dispatch(..., Ci.nsIThread.DISPATCH_NORMAL) with .dispatchToMainThread(...), r=froydnj.
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a26a27c192
provide a scriptable equivalent of NS_DispatchToMainThread, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd981920d0ef
script-generated patch to replace .{currentThread,mainThread}.dispatch(..., Ci.nsIThread.DISPATCH_NORMAL) with .dispatchToMainThread(...), r=froydnj.
Using try I narrowed the android failure to the changes in netwerk/test/httpserver/httpd.js. In comment 31 I relanded without the changes to that specific file.
Flags: needinfo?(florian)
https://hg.mozilla.org/mozilla-central/rev/d2a26a27c192
https://hg.mozilla.org/mozilla-central/rev/cd981920d0ef
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.