Closed Bug 1401641 Opened 2 years ago Closed 2 years ago
.49 - 13 .89% tp5o / tp5o _webext (osx-10-10) regression on push e4ac2e4268c7 (Tue Sep 19 2017)
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?
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 and perfherder. 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, so I've refrained from trying to land this here.  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eddd3edcbc95bc885efda8bd3e652ee5a1becc2&selectedJob=132396058  https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=2eddd3edcbc95bc885efda8bd3e652ee5a1becc2&framework=1&filter=osx&showOnlyImportant=0&selectedTimeRange=172800  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b75528a4d1ab1b50aa91acb0712c39cd7e8f222&selectedJob=132390331
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attachment #8910777 - Flags: review?(mstange)
:jmaher, could you take a look at perfherder 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!  https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=2eddd3edcbc95bc885efda8bd3e652ee5a1becc2&framework=1&filter=osx&showOnlyImportant=0&selectedTimeRange=172800
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 :)
2 years ago
Component: Untriaged → Widget: Cocoa
Product: Firefox → Core
Is this something that you are planning to uplift to 57? Thanks
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
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
You need to log in before you can comment on or make changes to this bug.