1463026 - DAMP regression on *.reload tests due to bug 1451592

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Brian Birtles (:birtles)]

See bug 1451592 comment 26:

> This patches regressed performance of many tests on DAMP:
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=try&originalRevision=703c14f284211de080c95cccf
> a317b8e2c8ffb76&newProject=try&newRevision=af1e9bad6918a2b04844f0ddf489c0e14f
> ff1438&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignatur
> e=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1
> 
> I think it is related to the call made to setToolboxButtons from
> `_updateFrames` as :pbro reported in bug 1455645.
> The concerning regressions are the one made to *.reload tests. The tools are
> slower to update after a page reload.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b68195394623f54e844103fe801615348b1ed08

Forthcoming perfherder results:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=9a813ef0884bfaaf538653b3a808e23c1cf557ac&newProject=try&newRevision=4b68195394623f54e844103fe801615348b1ed08&showOnlyImportant=1&framework=1

Comment 1

[Brian Birtles (:birtles)]

After digging into this a little bit my initial patch which just suppresses redundant changes turns out to be insufficient. That's because in the DAMP test we reload a page that must contain iframes (I haven't confirmed that part, but it's somewhat irrelevant considering the discussion below).

When we tear down the original instance of the page we get a series of frame-update events such as the following:

#1 frame-update { destroyAll: true }
#2 frame-update { <new parent frame> }
#3 frame-update { <old iframe>, destroy: true }
#4 frame-update { <new iframe> }
#5 frame-update { <new parent frame> } (Not sure what this is for. Updating the title perhaps?)

In #1 if we look to see if we have iframes we'll see we have none and so go to re-render and drop the button. However, once we get to #4 we'll see we have iframes and so we'll re-render to add the button.

As a result, in the reload case we end up dropping the button only to re-add it again. You can see this easily enough just by reloading a page with an iframe: there's a distinctive flicker when the page is reloaded.

It's easy to ignore the update in #1 by just looking for `destroyAll: true` but in #2 we'll face the same problem: no iframes means we should drop the button.

We can do some more complicated logic that avoids update if _either_ destroyAll is true OR it's a redundant change but the end result of that is we'll fail to drop the button when reloading a page causes it not to have any iframes (e.g. reloading attachment 8968176 [details]) since we'll do

#1 hasFrames: true -> false BUT destroyAll is true so no update
#2 (and beyond) hasFrames: false -> false so no update and iframes button lingers

In a sense, this is acting as expected. When we reload a page we drop iframes and so the button should be removed and only re-added after loading the page and checking that it does have iframes (which could take quite some time depending on network conditions).

It's unfortunate that this causes a DAMP regression, however. It's also somewhat unfortunate that it causes flicker when reloading simple pages.

To fix this we need to debounce updates to setToolboxButtons. We could do this with a generic timeout-based debounce which should fix the flicker for all cases but could cause tests to be unstable.

Alternatively we could try to debounce based on whether or not the parent iframe is loading or not. I'm a bit worried that this could produce a degraded experience for pages that take a long time to load or never completely load. We could try keying off DOMContentLoaded and see how that goes. Even if it fires before frame-update I think the .frames member of the page will still be populated such that _commandIsVisible returns true and we avoid flicker.

Either approach should have the side effect of fixing bug 1456788 however.

Comment 2

[Brian Birtles (:birtles)]

Thinking about the DOMContentLoaded approach to debouncing I'm concerned it would not work well for pages that take a long time to load (or perhaps never load completely in the HTTP streaming case). During that time the button would continue to show although clicking it might reveal an empty menu. Both from a performance point of view and polish point of view a timeout seems preferable.

It's really only testing that is complicated by this so I suggest we just make the tests robust enough to handle the timeout. We could also trigger the timeout behavior on a `destroyAll: true` such that it only applies to the reload case which might also mean less tests are affected.

Comment 3

[Brian Birtles (:birtles)]

After trying a few things it seems that a combination of debouncing and ignoring redundant changes fixes most of the perf regression:

1. Original regression due to call to setToolbox:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=d494933da96205b2f0ebe19395793a753bc888d6&newProject=try&newRevision=0fc71ac0167946ae3c1725eb149c05c9d783c466&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=1

2. With debouncing and ignoring redundant changes:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=d494933da96205b2f0ebe19395793a753bc888d6&newProject=try&newRevision=dcff75718d693693fd088eb3ffe6c3bfe73aaa4d&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=1

3. Comparing the change to status quo:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=0fc71ac0167946ae3c1725eb149c05c9d783c466&newProject=try&newRevision=dcff75718d693693fd088eb3ffe6c3bfe73aaa4d&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=1

I'm actually not quite sure why it doesn't fix it entirely but I think this brings it into an acceptable range.

Now to check if I get comparable results when applying a similar fix to the current trunk and with a shorter debounce interval.

Comment 4

[Brian Birtles (:birtles)]

Results seem to hold on current trunk:

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=846925f8c8382cdaad9c3d2b2d98ad2668399976&newProject=try&newRevision=9a085c0137abd36ba7433f8530c27295fb878e0c&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1

(The massive regression indicated on damp simple.netmonitor.reload.settle.DAMP appears to be bogus -- it wasn't there on the previous run despite using almost identical code and an historical graph of this result shows it often has these random spikes: https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800&series=try,1638962,1,1&highlightedRevisions=846925f8c838&highlightedRevisions=9a085c0137ab)

I've also confirmed that when reloading a page with iframes there are no calls to setToolboxButtons from _updateFrames with this patch applied.

Comment 5

[Brian Birtles (:birtles)]

Attachment: Bug 1463026 - Avoid unnecesarily setting the toolbox buttons when reloading the page;

Changeset https://hg.mozilla.org/mozilla-central/rev/00c968ee4e0c from
bug 1451592 introduced a DAMP regression on several reload tests. This is
because upon reloading a page we get a series of frame-update messages which
tell us that we no longer have any iframes only to then tell us we do have
iframes after all. As a result we update the toolbar buttons to remove the
frames button only to re-add it again. This causes unnecessary updates and
flicker.

This patch mitigates this problem by introducing debouncing during
reloading/navigating so that we only update the toolbar buttons after some
timeout of inactivity if there has been a change in the frames button's state.

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

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8980181 [details]
Bug 1463026 - Avoid unnecesarily setting the toolbox buttons when reloading the page;

https://reviewboard.mozilla.org/r/246342/#review252614

Thanks, this seems like a resonable approach to me! :)

::: devtools/client/framework/toolbox.js:2413
(Diff revision 1)
> +    // will be true), we should debounce the update to avoid unnecessary
> +    // flickering/rendering.
> +    if (data.destroyAll && !this.debouncedToolbarUpdate) {
> +      this.debouncedToolbarUpdate = debounce(() => {
> +        toolbarUpdate();
> +        this.debouncedToolbarUpdate = undefined;

Nit: I think it's more typical to clear with `null`, but at the same time it probably doesn't matter that much... Just making you aware! :)

Comment 7

[Brian Birtles (:birtles)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Nit: I think it's more typical to clear with `null`, but at the same time it
> probably doesn't matter that much... Just making you aware! :)

Oh, that's good to know. I was genuinely curious about that because although I used to set to null a lot, since switching to TypeScript (with --strictNullChecks) for a number of projects I've started setting to undefined since otherwise you either need to initialize all these values to null or else define their type as '<type> | undefined | null'. I've been wondering what sort of conventions others follow for this.

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8980181 [details]
Bug 1463026 - Avoid unnecesarily setting the toolbox buttons when reloading the page;

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d4723480e6c
Avoid unnecesarily setting the toolbox buttons when reloading the page; r=jryans

Comment 10

[Brian Birtles (:birtles)]

I intend to uplift this next week after it has baked on Nightly over the weekend.

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8d4723480e6c

Comment 12

[Brian Birtles (:birtles)]

Comment on attachment 8980181 [details]
Bug 1463026 - Avoid unnecesarily setting the toolbox buttons when reloading the page;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1451592
[User impact if declined]: Performance regression when reloading a page with DevTools inspector open
[Is this code covered by automated tests?]: The perf regression is and the feature affected is but there is no code specifically testing the added debouncing behavior per se.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not particularly.
[Why is the change risky/not risky?]: Fairly small and contained change to the timing of toolbar updates.
[String changes made/needed]: None.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8980181 [details]
Bug 1463026 - Avoid unnecesarily setting the toolbox buttons when reloading the page;

Fix for a DevTools perf regression introduced in 61. Approved for 61.0b9.

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1092f6f0d08d

Comment 15

[Alexandre Poirot [:ochameau]]

Thanks Brian!

Between -2.6% to -34% to most reload tests!

Looking at DAMP history, for simple.inspector.reload (which was one of the most regressed)
http://firefox-dev.tools/performance-dashboard/perfherder/?test=simple.inspector.reload&days=60&filterstddev=true&ignore-flags=true
We recovered from the original regression that happened on April 17th.

Comment 16

[Brian Birtles (:birtles)]

Thanks for monitoring the results! It's great when perf regression tests actually do what they're supposed to.