1437140 - Replace uses of NS_DispatchToMain/CurrentThread with more specific dispatching APIs

Status: RESOLVED FIXED

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

Description

[Josh Matthews [:jdm]]

I have a patch that replaces some more straightforward ones.

Comment 1

[Josh Matthews [:jdm]]

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

Comment 2

[Josh Matthews [:jdm]]

Attachment: Replace some uses of NS_DispatchToMainThread/NS_DispatchToCurrentThread with more specific event targets

Comment 3

[Dorel Luca [:dluca]]

Hello,

I was unable to apply the patch in order to push it to inbound due to the following error:

applying evenmoardocgroups
patching file dom/jsurl/nsJSProtocolHandler.cpp
Hunk #2 FAILED at 650
1 out of 2 hunks FAILED -- saving rejects to file dom/jsurl/nsJSProtocolHandler.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh evenmoardocgroups

:jdm, Can you please take a look at this?
Thanks

Comment 4

[Josh Matthews [:jdm]]

Attachment: Replace some uses of NS_DispatchToMainThread/NS_DispatchToCurrentThread with more specific event targets

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db673a3d83f9
Replace some uses of NS_DispatchToMainThread/NS_DispatchToCurrentThread with more specific event targets. r=mystor

Comment 6

[Noemi Erli[:noemi_erli]]

Backed out changeset db673a3d83f9 (bug 1437140) for speedometer crashes on a CLOSED TREE

Push with the failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a8aa7e2af130f28442a965188abdf80c30a737aa&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running

Log failure: https://treeherder.mozilla.org/logviewer.html#?job_id=162445372&repo=mozilla-inbound&lineNumber=1198

Backout push: https://hg.mozilla.org/integration/mozilla-inbound/rev/24f191972279a7fcac58aaa55d6f85a08a4ef115

Comment 7

[Josh Matthews [:jdm]]

Attachment: Replace some uses of NS_DispatchToMainThread/NS_DispatchToCurrentThread with more specific event targets

Now using the infallible OwnerDocument() instead of the nullable aDocument.

Comment 8

[Josh Matthews [:jdm]]

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

Comment 9

[Josh Matthews [:jdm]]

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

Comment 10

[Josh Matthews [:jdm]]

Attachment: Replace some uses of NS_DispatchToMainThread/NS_DispatchToCurrentThread with more specific event targets

Comment 11

[Ionuț Goldan [:igoldan]]

When this landed (comment 5), this performance regression was noticed:

== Change summary for alert #11587 (as of Thu, 15 Feb 2018 14:00:42 GMT) ==

Regressions:

  2%  tp5o linux64 pgo e10s stylo     138.62 -> 141.93

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11587

Comment 12

[Ionuț Goldan [:igoldan]]

The back out resolved it.

Note that the perf regression above happened only on Linux, on PGO build only. This makes me doubt there's something real about it. Unless you know otherwise, :jdm?

Comment 13

[Ionuț Goldan [:igoldan]]

The landing from comment 5 also showed a reduction in the installer size; again, we lost it after the backout.

== Change summary for alert #11605 (as of Fri, 16 Feb 2018 05:11:25 GMT) ==

Improvements:

  1%  installer size linux32 pgo      64,460,160.54 -> 63,821,899.92
  0.4%  installer size linux64 pgo      63,677,511.33 -> 63,405,577.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11605

Comment 14

[Ionuț Goldan [:igoldan]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #11)
> When this landed (comment 5), this performance regression was noticed:
> 
> == Change summary for alert #11587 (as of Thu, 15 Feb 2018 14:00:42 GMT) ==
> 
> Regressions:
> 
>   2%  tp5o linux64 pgo e10s stylo     138.62 -> 141.93
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=11587

This is the expected perf improvement after the backout:

== Change summary for alert #11604 (as of Thu, 15 Feb 2018 16:53:47 GMT) ==

Improvements:

  2%  tp5o linux64 pgo e10s stylo     141.86 -> 139.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11604

Comment 15

[Bobby Holley (:bholley)]

It looks like the patch was backed out for causing speedometer crashes on linux. If I recall correctly, we started running speedometer as part of the PGO training run in bug 1356652. So what probably happened is that this patch caused a content process crash on speedometer, depriving the PGO build of profile data. This in turn caused it to do less inlining, which improved codesize at the expense of performance.

So if the relanded patch fixes the speedometer crashes, I'd also expect it the Tp/codesize changes to go away.

Comment 16

[Josh Matthews [:jdm]]

That's a plausible theory.

Comment 17

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

That bitrotted, please provide an updated patch.

Comment 18

[Josh Matthews [:jdm]]

Attachment: Replace some uses of NS_DispatchToMainThread/NS_DispatchToCurrentThread with more specific event targets

Comment 19

[Josh Matthews [:jdm]]

Attachment: Replace some uses of NS_DispatchToMainThread/NS_DispatchToCurrentThread with more specific event targets

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e14ee07de8a
Replace some uses of NS_DispatchToMainThread/NS_DispatchToCurrentThread with more specific event targets. r=mystor

Comment 21

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2e14ee07de8a