Closed Bug 1459538 Opened 2 years ago Closed 2 years ago

5.61 - 8.72% damp (linux64, osx-10-10, windows10-64, windows7-32) regression on push 18937f7c5c2fe8d38b7e479c18f81816368df503 (Thu May 3 2018)

Categories

(DevTools :: Inspector, defect, P1)

Unspecified
All
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61+ fixed, firefox62+ fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 + fixed
firefox62 + fixed

People

(Reporter: igoldan, Assigned: gl)

References

Details

(Keywords: perf, regression, talos-regression)

Attachments

(1 file)

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=18937f7c5c2fe8d38b7e479c18f81816368df503

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  9%  damp osx-10-10 opt e10s stylo     109.74 -> 119.32
  6%  damp windows7-32 pgo e10s stylo   69.26 -> 73.73
  6%  damp windows10-64 opt e10s stylo  71.78 -> 76.37
  6%  damp linux64 opt e10s stylo       63.40 -> 67.18
  6%  damp windows10-64 pgo e10s stylo  67.45 -> 71.45
  6%  damp linux64 pgo e10s stylo       60.44 -> 63.83


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=13059

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
:gl Looks like bug 1446944 caused big DAMP perf regressions. Is there something we can do to fix or at least lessen this. Or should we rather accept the tooltip penalties?
Flags: needinfo?(gl)
This change made all the settle tests increase significantly, because we trigger the display of this onboarding tooltip on clean profiles. Showing a tooltip is async and we don't wait on the completion of the show to consider the inspector as initialized (waiting on it would not change the actual regression, but would move the load to the "show" tests rather than the settle tests).

Given that this tooltip should not be displayed in most of the runs I think we could simply disable it for our DAMP tests.
Alex: what is your opinion about this?
Flags: needinfo?(poirot.alex)
Ahah 20,528.36% regression, nice Gabriel :)

Yes, preffing this off sounds good to me.

We already do this for the new console frontend over here:
  https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#83
We may want to make this more generic/discoverable.


Note the existence of this pref file used by all mochitest suites:
  https://searchfox.org/mozilla-central/source/testing/profiles/common/user.js#31-37
  (it may have been better to pref it off from this instead of devtools/client/shared/test/shared-head.js)
  (unfortunately, this isn't shared with xpcshell)
But for DAMP, we want another pref list as we don't want the logging prefs turned on.
Flags: needinfo?(poirot.alex)
Assignee: nobody → gl
Status: NEW → ASSIGNED
Flags: needinfo?(gl)
Comment on attachment 8973662 [details]
Bug 1459538 - Pref off the 3 pane inspector onboarding tooltip for DAMP.

https://reviewboard.mozilla.org/r/242028/#review247890

::: testing/talos/talos/tests/devtools/addon/content/damp.js:84
(Diff revision 1)
>  
>  /* globals res:true */
>  
>  function Damp() {
>    Services.prefs.setBoolPref("devtools.webconsole.new-frontend-enabled", true);
> +  Services.prefs.setBoolPref("devtools.inspector.show-three-pane-tooltip", false);

Could you add a comment about why we disable just this? (may be a link to the bug would help)

And a Talos run would help confirming that it fixes the regression before landing it :)
Attachment #8973662 - Flags: review?(poirot.alex) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c66986ae29bc
Pref off the 3 pane inspector onboarding tooltip for DAMP. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/c66986ae29bc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Target Milestone: Firefox 61 → Firefox 62
We can uplift this to Beta a=test-only. Thanks for the quick fix, Gabriel!
Whiteboard: [checkin-needed-beta]
I confirm this fixed the regressions:

== Change summary for alert #13115 (as of Mon, 07 May 2018 16:04:01 GMT) ==

Improvements:

 10%  damp osx-10-10 opt e10s stylo     123.05 -> 110.88
  7%  damp windows10-64 opt e10s stylo  78.75 -> 73.10
  6%  damp linux64 opt e10s stylo       69.60 -> 65.10
  6%  damp linux64 pgo e10s stylo       65.48 -> 61.48
  5%  damp windows7-32 pgo e10s stylo   75.01 -> 71.12

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13115
Product: Firefox → DevTools
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.