Closed Bug 1401641 Opened 4 years ago Closed 4 years ago

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


(Core :: Widget: Cocoa, defect)

53 Branch
Not set



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


(Reporter: jmaher, Assigned: spohl)



(Keywords: perf, regression, talos-regression)


(1 file)

Talos has detected a Firefox performance regression from push:

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


 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:

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:

For information on reproducing and debugging the regression, either on try or locally, see:

*** 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:
: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.

Assignee: nobody → spohl.mozilla.bugs
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!

Flags: needinfo?(jmaher)
Attachment #8910777 - Flags: review?(mstange) → review+
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
Closed: 4 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]

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]

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) ==


 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:
Looks like this also improved DAMP:

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


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

For up to date results, see:
Blocks: 1404036
No longer blocks: 1404036
You need to log in before you can comment on or make changes to this bug.