Closed
Bug 1401682
Opened 8 years ago
Closed 8 years ago
2.29 - 6.38% tart (linux64, windows7-32) regression on push 2ecda1649fe43924e185445db870d7fd7e27bb0b (Wed Sep 20 2017)
Categories
(Firefox :: New Tab Page, defect, P2)
Tracking
()
VERIFIED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | fixed |
firefox58 | --- | fixed |
People
(Reporter: jmaher, Assigned: Mardak)
References
Details
(Keywords: perf, regression, talos-regression)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
k88hudson
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
Talos has detected a Firefox performance regression from push:
https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=2ecda1649fe43924e185445db870d7fd7e27bb0b
As author of one of the patches included in that push, we need your help to address this regression.
Regressions:
6% tart summary linux64 pgo e10s 3.75 -> 3.99
5% tart summary linux64 opt e10s 4.12 -> 4.34
2% tart summary windows7-32 pgo e10s 4.28 -> 4.38
You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=9562
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
Reporter | ||
Comment 1•8 years ago
|
||
:Mardak, can you take a look at this regression?
Flags: needinfo?(edilee)
Assignee | ||
Comment 2•8 years ago
|
||
That export did contain the fading. And tart does open new tabs fast enough to not be preloaded (but is prerendered). I would think animating fade could compete with the tab animating time.
Flags: needinfo?(edilee) → needinfo?(khudson)
Assignee | ||
Comment 3•8 years ago
|
||
Looks like it's the transition:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=319a34bea9e4&newProject=try&newRevision=36ad8c34596843afc175587bc2612338e51e1377&framework=1&filter=tart%2032%20opt&showOnlyImportant=0
I backed out the one line `- transition: opacity 0.1s ease-in-out; }`
win32 opt tart improves by 2%.
Updated•8 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox57:
--- → affected
Component: Untriaged → Activity Streams: Newtab
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → edilee
Flags: needinfo?(khudson)
Version: 53 Branch → 57 Branch
Assignee | ||
Comment 4•8 years ago
|
||
After playing around with various transition timings. Here's the results for various duration / delay in ms for linux64 opt:
100/ 10 1.91% regress
100/ 50 2.05% regress
100/100 -2.23%
100/110 -3.57%
50/115 -3.98%
75/115 -3.98%
100/115 -4.49%
100/120 -4.72%
50/125 -5.09%
75/125 -4.90%
100/125 -4.86%
100/130 -5.09%
100/150 -5.13%
100/250 -5.27%
100/500 -4.49%
100/1.s -5.23%
100/2.s -4.90%
100/5.s -5.09%
These are for 5 runs, so there's some noise but basically anything around 5% ± 0.2% improvement seems to indicate a full restoration of tart avoiding the regression for this bug.
Probably good to note that the `Linux x64 QuantumRender opt` were practically unchanged for any of those values, so we should be able to revert the fix here with QuantumRender enabled.
Checking with UX on acceptable transitions.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
k88hudson, if this is good, we can land this on autoland and separately backport the fix with more comments, etc.
Assignee | ||
Comment 7•8 years ago
|
||
Alternatively, we can just fix this as part of a usual export. https://github.com/mozilla/activity-stream/pull/3571/files
Updated•8 years ago
|
status-firefox58:
--- → affected
Priority: -- → P2
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8910940 [details]
Bug 1401682 - Adjust activity stream fade in timing to avoid tart regression.
https://reviewboard.mozilla.org/r/182396/#review187938
Nice!
Attachment #8910940 -
Flags: review?(khudson) → review+
Pushed by khudson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66f49bef247d
Adjust activity stream fade in timing to avoid tart regression. r=k88hudson
Comment 10•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Assignee | ||
Comment 11•8 years ago
|
||
Just to confirm, this did improve tart 6%:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=autoland&originalRevision=754a0d9a44ea&newProject=autoland&newRevision=66f49bef247d&framework=1
Maybe no alert for the improvement because bug 1397918 had a measured ~40% tart regression:
https://treeherder.mozilla.org/perf.html#/alerts?id=9621
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8910940 [details]
Bug 1401682 - Adjust activity stream fade in timing to avoid tart regression.
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1401418 export that added the fade
[User impact if declined]: Slower tab opening as measured by talos tart
[Is this code covered by automated tests?]: No
[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?]: Not really
[Why is the change risky/not risky?]: 1 line css change adjusting times
[String changes made/needed]: None
Attachment #8910940 -
Flags: approval-mozilla-beta?
Comment 13•8 years ago
|
||
Could be pushed to FF57?
Updated•8 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 14•8 years ago
|
||
Comment on attachment 8910940 [details]
Bug 1401682 - Adjust activity stream fade in timing to avoid tart regression.
Fix a trivial perf regression, taking it.
Should be in 57b3
Attachment #8910940 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 15•8 years ago
|
||
bugherder uplift |
Comment 16•8 years ago
|
||
(In reply to Ed Lee :Mardak from comment #11)
> Maybe no alert for the improvement because bug 1397918 had a measured ~40%
> tart regression:
> https://treeherder.mozilla.org/perf.html#/alerts?id=9621
Actually, we did receive improvements notifications. They look very good:
== Change summary for alert #9649 (as of September 22 2017 14:22 UTC) ==
Improvements:
6% tart summary linux64 opt e10s 5.99 -> 5.62
2% tart summary windows7-32 pgo e10s 4.32 -> 4.22
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9649
Updated•6 years ago
|
Component: Activity Streams: Newtab → New Tab Page
You need to log in
before you can comment on or make changes to this bug.
Description
•