Closed Bug 1463026 Opened Last year Closed Last year

DAMP regression on *.reload tests due to bug 1451592

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox61 fixed, firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
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.
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.
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.
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 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! :)
Attachment #8980181 - Flags: review?(jryans) → review+
(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.
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d4723480e6c
Avoid unnecesarily setting the toolbox buttons when reloading the page; r=jryans
I intend to uplift this next week after it has baked on Nightly over the weekend.
https://hg.mozilla.org/mozilla-central/rev/8d4723480e6c
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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.
Attachment #8980181 - Flags: approval-mozilla-beta?
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.
Attachment #8980181 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Thanks for monitoring the results! It's great when perf regression tests actually do what they're supposed to.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.