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

Status: RESOLVED FIXED

(Firefox :: Untriaged, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 1

[Julian Descottes [:jdescottes]]

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.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1346781 - do not attempt to hide highlighters if currentNode is falsy;

Review commit: https://reviewboard.mozilla.org/r/119816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119816/

Comment 3

[Julian Descottes [:jdescottes]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119816/diff/1-2/

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

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)
```

Comment 5

[Julian Descottes [:jdescottes]]

Thanks for the review! Updated the patch accordingly, I'll wait for the other changeset to reach central before landing.

Comment 6

[Julian Descottes [:jdescottes]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119816/diff/2-3/

Comment 7

[Julian Descottes [:jdescottes]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119816/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9555f7ced9de
do not attempt to hide highlighters if currentNode is falsy;r=zer0

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9555f7ced9de

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

thanks, verified on the graph!