Closed
Bug 1307365
Opened 8 years ago
Closed 8 years ago
2.5 - 4.48% tpaint (osx-10-10, windows7-32, windows8-64) regression on push d271311721bbfad7128b6ce73e23569059deae03 (Thu Sep 29 2016)
Categories
(DevTools :: General, defect)
Tracking
(firefox49 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox49 | --- | unaffected |
firefox50 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | --- | fixed |
People
(Reporter: ashiue, Unassigned)
References
Details
(Keywords: perf, regression, talos-regression)
Talos has detected a Firefox performance regression from push d271311721bbfad7128b6ce73e23569059deae03. As author of one of the patches included in that push, we need your help to address this regression.
Regressions:
4% tpaint windows7-32 opt e10s 289.4 -> 302.36
4% tpaint windows8-64 opt e10s 275.71 -> 287.33
3% tpaint osx-10-10 opt e10s 310.28 -> 318.05
You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=3514
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
|
||
I did some retriggers, here is the better zooming view:
https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bautoland,6b31d18100b143c31595be90e6f8bb46b64bb31b,1,1%5D&series=%5Bautoland,1246a0dbc2c4bc48fda93ec9f956a545db993460,1,1%5D&series=%5Bautoland,83892f58324732a38c5aefbe7fa251143d4cf7bb,1,1%5D&zoom=1475125494729.205,1475185438771.719,260.2996254681648,328.314606741573
Hi Alexandre, as you are the patch author, can you take a look at this and determine what is the root cause? Thanks!
Comment 2•8 years ago
|
||
Hum. I already got backed out and tried to assert that my patch wasn't regressing...
See bug 1297535 comment 21.
https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=cfdb7af3af2e&newProject=try&newRevision=e5e8ad106067&framework=1&filter=tabpaint&showOnlyImportant=0
I've only ran test on Windows 32, but that didn't exposed any regression back then.
This wasn't testing exactly the patch that landed, but I don't see any difference that would make the startup slower.
Do you have any idea why my report would not report a regression while you are reporting one here?
Is there anything wrong with my try push and perf tool usage??
Any difference between my try push and yours? (except that your are running on more platforms)
Flags: needinfo?(poirot.alex)
Comment 3•8 years ago
|
||
interesting. I think you did the try push and compare view correct. The regression is in tpaint (vs tabpaint)- oh I love how the names have become so similar over the years.
looking at tpaint:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=cfdb7af3af2e&newProject=try&newRevision=e5e8ad106067&framework=1&filter=tpaint%20opt%20e10s&showOnlyImportant=0
you can see a 1.93% regression on win7, still no big red flags, not the 4% we see in the report.
I noticed the base revision showed a lower value (278) than what we see when this landed (289). Possibly the changes that took place for this test between try push and landing (roughly 2 weeks) tickled something in the patch?
This only affects tpaint e10s, not sure why it doesn't affect non e10s- maybe that is a place to look?
Comment 4•8 years ago
|
||
The regression itself is quite obvious and I'm afraid it is going to be hard to not regress.
This patch starts loading some devtools modules earlier in Firefox startup and create some XUL elements earlier. It isn't introducing new operation, it is just a shift in time.
My issue here is that talos looks unreliable. Pushing to try doesn't seem really helpful.
So that it makes it even harder to try to optimize this patch that I know is going to regress anyway.
Hopefully we can figure something to make the regression as small as possible.
The best would be to able to assert the regression locally. That would really help me figure out something... But given results on try, I'm not sure it will be any better locally.
Comment 5•8 years ago
|
||
Let just backout the offending patch. I'm not confident I can fix this patch to accomodate talos.
Comment 6•8 years ago
|
||
Before we back this out, I think we should try and assess what's best for the users considering what we know.
We know there's a relatively small penalty on few platforms in opening a new window. On its own, it's unlikely that users would notice it, but these penalties do add up eventually.
One benefit for users is not displaying a console error message, which bug 1297535 addresses. Are there other benefits too?
Is the approach which bug 1297535 takes a reasonable one? the easiest? the best considering $other_factors?
Etc.
In general there's a clear tradeoff between doing stuff lazily with some possible latency when the user wants it, vs doing a stuff initially and getting more responsiveness later, but with possibly longer initialization and possibly never using it later.
Comment 7•8 years ago
|
||
(In reply to Avi Halachmi (:avih) from comment #6)
> Before we back this out, I think we should try and assess what's best for
> the users considering what we know.
>
> We know there's a relatively small penalty on few platforms in opening a new
> window. On its own, it's unlikely that users would notice it, but these
> penalties do add up eventually.
>
> One benefit for users is not displaying a console error message, which bug
> 1297535 addresses. Are there other benefits too?
If prevent the error message and fix the tooltip message on the WebIDE icon on the newly opened windows. Nothing more.
>
> Is the approach which bug 1297535 takes a reasonable one? the easiest? the
> best considering $other_factors?
I tried to take the most obvious one between being technically correct and not working around the original issue.
> Etc.
>
> In general there's a clear tradeoff between doing stuff lazily with some
> possible latency when the user wants it, vs doing a stuff initially and
> getting more responsiveness later, but with possibly longer initialization
> and possibly never using it later.
I consider any perf tradeoff introduced by devtools for all users to be very bad.
And that's potentially the case here. This slowness is to be paid by everyone, not just firefox users who are using devtools.
Component: Untriaged → Developer Tools
status-firefox49:
--- → unaffected
status-firefox50:
--- → unaffected
status-firefox51:
--- → unaffected
Comment 8•8 years ago
|
||
Alison, Could you please backout this patch?
I'm about to have a better alternative in bug 1297535.
Flags: needinfo?(ashiue)
Comment 9•8 years ago
|
||
Hi Alexandre,
I've backed out your push in https://bugzilla.mozilla.org/show_bug.cgi?id=1297535#c50.
Let me know if you have any questions.
Flags: needinfo?(ashiue) → needinfo?(poirot.alex)
Comment 10•8 years ago
|
||
Thanks, I'll pursue this fix in bug 1297535.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(poirot.alex)
Resolution: --- → FIXED
Comment 11•8 years ago
|
||
marking fixed in 52 since this was backed out
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•