Closed Bug 1401641 Opened 7 years ago Closed 7 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)

Product:
Core
Component:
Widget: Cocoa
Version:
53 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
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)

Patch
1.13 KB, patch
mstange
: review+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
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 Patch — — Splinter 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)
tracking-firefox57: --- → ?
: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: 7 years ago
status-firefox58: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: