1.41% damp (linux64) regression on push 4a9ad284a59665dab2889370800b1f45a1d784bc (Fri Mar 10 2017)

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Untriaged
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jmaher, Assigned: jdescottes)

Tracking

(Blocks: 1 bug, {perf, regression, talos-regression})

53 Branch
Firefox 55
perf, regression, talos-regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Talos has detected a Firefox performance regression from push 4a9ad284a59665dab2889370800b1f45a1d784bc. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  1%  damp summary linux64 opt e10s     317.8 -> 322.28


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

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

* removing the 3 day window as this is <2%, but probably worth documenting as this is a 1.4% regression
(Reporter)

Updated

a year ago
Blocks: 1346783
Thanks for filing.

It looks like the patch made the inspector close twice as slow in the "complicated" case (damp complicated.inspector.close.DAMP).

If a grid highlighter was displayed, we now save the selector for the currently highlighted element. However that shouldn't have any impact when no highlighter is used.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
Comment on attachment 8846816 [details]
Bug 1346781 - do not attempt to hide highlighters if currentNode is falsy;

https://reviewboard.mozilla.org/r/119816/#review121724

::: devtools/server/actors/highlighters/auto-refresh.js:122
(Diff revision 2)
>  
>    /**
>     * Hide the highlighter
>     */
>    hide: function () {
> -    if (Cu.isDeadWrapper(this.highlighterEnv.window)) {
> +    if (!this.currentNode || Cu.isDeadWrapper(this.highlighterEnv.window)) {

That's make sense, if we attempt to call `hide` when `show` was never called in first place.
Notice that the `window` getter is changed now, so you want probably to build your patch on top of this one: https://hg.mozilla.org/integration/autoland/rev/b1b91f7861ec

And having something like:

```js
if (!this.currentNode || !this.highlighterEnv.window)
```
Attachment #8846816 - Flags: review?(zer0) → review+
Thanks for the review! Updated the patch accordingly, I'll wait for the other changeset to reach central before landing.
Comment hidden (mozreview-request)
Depends on: 1327725

Updated

a year ago
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
Comment hidden (mozreview-request)

Comment 8

a year ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9555f7ced9de
do not attempt to hide highlighters if currentNode is falsy;r=zer0

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9555f7ced9de
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Reporter)

Comment 10

a year ago
thanks, verified on the graph!
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.