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)

52 Branch
defect
Not set
normal

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
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)
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?
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.
Let just backout the offending patch. I'm not confident I can fix this patch to accomodate talos.
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.
(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
Alison, Could you please backout this patch? I'm about to have a better alternative in bug 1297535.
Flags: needinfo?(ashiue)
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)
Thanks, I'll pursue this fix in bug 1297535.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(poirot.alex)
Resolution: --- → FIXED
marking fixed in 52 since this was backed out
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.