Closed Bug 1401641 Opened 2 years ago Closed 2 years ago

6.49 - 13.89% tp5o / tp5o_webext (osx-10-10) regression on push e4ac2e4268c7 (Tue Sep 19 2017)

Categories

(Core :: Widget: Cocoa, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: jmaher, Assigned: spohl)

References

Details

(Keywords: perf, regression, talos-regression)

Attachments

(1 file)

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=e4ac2e4268c7

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

 14%  tp5o_webext summary osx-10-10 opt e10s     401.72 -> 457.52
  6%  tp5o summary osx-10-10 opt e10s            262.73 -> 279.78


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=9559

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Buildbot/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling
:spohl, I see you wrote the patch which caused this regression, can you take a look at this and help determine a course of action?
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch PatchSplinter Review
It turns out that the [mWindow disableSetNeedsDisplay] and [mWindow enableSetNeedsDisplay] calls in nsCocoaWindow::SetTitle were indeed helping performance, unlike what I wrote in bug 1398582 comment 28. In my local testing I usually opened new windows to test things, and that's where we would fail to call [mWindow disableSetNeedsDisplay] and [mWindow enableSetNeedsDisplay] because mDrawsIntoWindowFrame was still set to NO. However, if a user opened and closed *tabs*, this would indeed get called because mDrawsIntoWindowFrame will have been set to YES by that point. It looks like our Talos tests are exercising opening & closing tabs quite a bit. Try run[1] and perfherder[2].

There may be further performance gains to be had from changing initWithContentRect: to initialize mDrawsIntoWindowFrame to YES by default. This could improve performance when opening new windows, as we will now be calling [mWindow disableSetNeedsDisplay] and [mWindow enableSetNeedsDisplay] there as well. However, there are failures on try when doing so[3], so I've refrained from trying to land this here.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eddd3edcbc95bc885efda8bd3e652ee5a1becc2&selectedJob=132396058
[2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=2eddd3edcbc95bc885efda8bd3e652ee5a1becc2&framework=1&filter=osx&showOnlyImportant=0&selectedTimeRange=172800
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b75528a4d1ab1b50aa91acb0712c39cd7e8f222&selectedJob=132390331
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8910777 - Flags: review?(mstange)
:jmaher, could you take a look at perfherder[1] and see if this fully resolves the performance regressions? This will most likely be the best we can do without having significant visual artifacts on macOS 10.13. Thank you!

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=2eddd3edcbc95bc885efda8bd3e652ee5a1becc2&framework=1&filter=osx&showOnlyImportant=0&selectedTimeRange=172800
Flags: needinfo?(jmaher)
Attachment #8910777 - Flags: review?(mstange) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2abc46cea00b3a44f2a0118fbd6851024290399b
Bug 1401641: Avoid invalidations on macOS when setting window titles when titles aren't being displayed. r=mstange
this fix looks really great!  thanks for doing this work :)
Flags: needinfo?(jmaher)
Component: Untriaged → Widget: Cocoa
Product: Firefox → Core
https://hg.mozilla.org/mozilla-central/rev/2abc46cea00b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Is this something that you are planning to uplift to 57? Thanks
Flags: needinfo?(spohl.mozilla.bugs)
Comment on attachment 8910777 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1398582
[User impact if declined]: Performance regression
[Is this code covered by automated tests?]: Yes (Talos)
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This change is restoring previously existing code that shouldn't have been removed in bug 1398582.
[String changes made/needed]: None
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8910777 - Flags: approval-mozilla-beta?
Comment on attachment 8910777 [details] [diff] [review]
Patch

Fix a perf regression, taking it.
Should be in 57 beta 3
Attachment #8910777 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Confirming the fix:

== Change summary for alert #9613 (as of September 21 2017 19:32 UTC) ==

Improvements:

 12%  tp5o_webext summary osx-10-10 opt e10s     455.66 -> 401.70
  7%  tp5o summary osx-10-10 opt e10s            280.83 -> 262.50

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9613
Looks like this also improved DAMP:

== Change summary for alert #9653 (as of September 26 2017 05:58 UTC) ==

Improvements:

  3%  damp summary osx-10-10 opt e10s     296.00 -> 286.49

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9653
Blocks: 1404036
No longer blocks: 1404036
You need to log in before you can comment on or make changes to this bug.