Closed Bug 1437140 Opened 4 years ago Closed 4 years ago

Replace uses of NS_DispatchToMain/CurrentThread with more specific dispatching APIs

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jdm, Assigned: jdm)

Details

Attachments

(1 file, 5 obsolete files)

I have a patch that replaces some more straightforward ones.
Assignee: nobody → josh
Status: NEW → ASSIGNED
Attachment #8949830 - Flags: review?(nika) → review+
Keywords: checkin-needed
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
Flags: needinfo?(josh)
Attachment #8949830 - Attachment is obsolete: true
Flags: needinfo?(josh)
Keywords: checkin-needed
Priority: -- → P2
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db673a3d83f9
Replace some uses of NS_DispatchToMainThread/NS_DispatchToCurrentThread with more specific event targets. r=mystor
Keywords: checkin-needed
Now using the infallible OwnerDocument() instead of the nullable aDocument.
Attachment #8950242 - Attachment is obsolete: true
Attachment #8951448 - Attachment is obsolete: true
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
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?
Flags: needinfo?(josh)
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
(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
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.
That's a plausible theory.
Flags: needinfo?(josh)
Keywords: checkin-needed
That bitrotted, please provide an updated patch.
Flags: needinfo?(josh)
Keywords: checkin-needed
Attachment #8951502 - Attachment is obsolete: true
Flags: needinfo?(josh)
Keywords: checkin-needed
Attachment #8952368 - Attachment is obsolete: true
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e14ee07de8a
Replace some uses of NS_DispatchToMainThread/NS_DispatchToCurrentThread with more specific event targets. r=mystor
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e14ee07de8a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.