1355161 - Expose a scriptable equivalent of NS_DispatchToMainThread

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Florian Quèze [:florian]]

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?

Comment 1

[(no longer active)]

Yes, using executeSoon() would be more efficient.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Would the module "Firefox:General" be a better fit here?

Comment 3

[Florian Quèze [:florian]]

(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.

Comment 4

[Andrew Overholt [:overholt]]

(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?

Comment 5

[Andreas Farre [:farre]]

(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.

Comment 6

[Florian Quèze [:florian]]

(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.

Comment 7

[Florian Quèze [:florian]]

Attachment: make nsIEventTarget.dispatch's flag parameter optional

The interface change proposed in comment 6.

Comment 8

[Nathan Froyd [:froydnj]]

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 9

[Nathan Froyd [:froydnj]]

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!

Comment 10

[Florian Quèze [:florian]]

(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 :).

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Quantum DOM is for child processes only.

Comment 12

[(no longer active)]

(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.

Comment 13

[(no longer active)]

Marking qf- since this patch in itself doesn't improve performance.  ;-)

Comment 14

[Florian Quèze [:florian]]

Attachment: dispatchToMainThread WIP

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?

Comment 15

[Nathan Froyd [:froydnj]]

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?

Comment 16

[Florian Quèze [:florian]]

Attachment: Add dispatchToMainThread, v2

Comment 17

[Nathan Froyd [:froydnj]]

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.

Comment 18

[Florian Quèze [:florian]]

Attachment: script-generated cleanup patch

Comment 19

[Florian Quèze [:florian]]

Attachment: xpcshell script

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8858093 [details] [diff] [review]
script-generated cleanup patch

Nathan said he can review this.

Comment 21

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5f8f1aca39fb194dfd2c8c8ce6249122af48049

Comment 22

[Florian Quèze [:florian]]

Rephrasing the bug summary, as the setTimeout replacement will likely happen in another bug at this point.

Comment 23

[Florian Quèze [:florian]]

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.

Comment 24

[Florian Quèze [:florian]]

Attachment: script-generated patch, v2

Comment 25

[Florian Quèze [:florian]]

Attachment: xpcshell script v2

The location information of the first argument wasn't reliable, this now uses the callee's location instead.

Comment 26

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f849ddc95fdf489871b4dd374ad92d8983ebc299

Comment 27

[Florian Quèze [:florian]]

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.

Comment 28

[Nathan Froyd [:froydnj]]

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.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for supposedly breaking reftests and crashtests on Android 4.3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/41e323a5010edd33bf16f67db5da60583b457eb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d95e9e35034405215f1db9e21cfd36b403669d60

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=18d45aa984d6eb1bc0ce210e1c105a6b9e573b5f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
These tests fail but then are stuck and time out.

Also had to back out bug 1356569 to be able to back out this one.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 32

[Florian Quèze [:florian]]

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.

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d2a26a27c192
https://hg.mozilla.org/mozilla-central/rev/cd981920d0ef