1401187 - DAMP tool's reload probe only measure tab reload whereas it should also wait for tool reload

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

http://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#52-64

For now, it only measure the time it takes for "load" event to fire.
It will surely measure the impact on document load time,
but won't track how much time it takes to actually process the new page in the tools.

We should wait for some toolbox or tool event to measure how much times it takes to handle the new page in each tool.

Comment 1

[Brian Grinstead [:bgrins]]

It was done that way originally to measure the impact that the tool had on browser content. With e10s it's quite possible that this number isn't as relevant (although it does appear that some tools do cause the page to reload more slowly and there have been bug reports indicating this as well)

But, I don't think that we have a uniform definition of 'reloading' for each tool - would need to specify a what point exactly each tool finishes reacting a page reload. For instance, in the console I suppose the reload is done once the output from the first window is cleared? Messages for the reloaded window will come in arbitrarily as scripts execute - it's not clear to me how long we should wait for new messages before emitting the event.

Comment 2

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> It was done that way originally to measure the impact that the tool had on
> browser content. With e10s it's quite possible that this number isn't as
> relevant (although it does appear that some tools do cause the page to
> reload more slowly and there have been bug reports indicating this as well)

It complely hides the very significant regression debugger had which are related to bug 1399242 profile.
DAMP only report 2-4% perf win when fixing very significant performance issues and even qualify it with "med" confidence.
Also, this bug may not explain everything, may be bild.de isn't the best site to use.
It doesn't reflect the "very special" experience you get against webpack apps!
May be we should also have 3 documents: simple, medium and complicated.

> But, I don't think that we have a uniform definition of 'reloading' for each
> tool - would need to specify a what point exactly each tool finishes
> reacting a page reload. For instance, in the console I suppose the reload is
> done once the output from the first window is cleared? Messages for the
> reloaded window will come in arbitrarily as scripts execute - it's not clear
> to me how long we should wait for new messages before emitting the event.

Yes, it may be hard, but at this stage, anything we do, even very naive, would be already a significant step forward!

For example, we could:
* wait for arbitrary number of messages (console) or sources (debugger) for simple and complicated,
  This would make DAMP require more maintenance, but should be straightforward and produce valid measures.
* try to use a more generic "settle" function before ending the measure,
  This would introduce a virtual overhead, but hopefully the same one to all runs?
* ...?

Comment 3

[Alexandre Poirot [:ochameau]]

Given your feedback on bug 1394804, I think I made the good call.
But note that while writing this patch, I was wondering if I should keep the existing "reload" tests as-is.
Here, I'm modifying them in order to track tools loading instead of page loading.
We may want to keep track of both? We may want to still know how much we impact page load?

Locally, I notice a very big difference between old and new reload measures, let's see what talos says on CI:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=27de12e0a2c461937ce673a247a4b0914db218d8

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1401187 - Consider tool reload when reloading web page in DAMP.

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

Comment 5

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=6bcbf9d8a4ce&newProject=try&newRevision=27de12e0a2c461937ce673a247a4b0914db218d8&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1
It makes a significant bump for inspector, console and debugger "reload" tests, while reducing "close" tests, which were processed while reload was still going on.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8910020 [details]
Bug 1401187 - Consider tool reload when reloading web page in DAMP.

https://reviewboard.mozilla.org/r/181492/#review187320

Thanks, this looks good. Would it be easy to add a reload function for the netmonitor? Can be done in another bug if you prefer

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1401187 - Update and sign DAMP add-on.

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

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8910020 [details]
Bug 1401187 - Consider tool reload when reloading web page in DAMP.

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

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8910671 [details]
Bug 1401187 - Update and sign DAMP add-on.

https://reviewboard.mozilla.org/r/182114/#review187468

Comment 10

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=714ef58679c8f0ec4c35c90e04e068acff4ef90f&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800
One last try, this is moving a lot of computation from close to reload tests.
Overall it seems to also increase the summary.

Performance sheriffs, please expect a change in damp summary.
This change is increasing the scope of all reload tests and so will most likely increase the overall values.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae2699e5b050
Consider tool reload when reloading web page in DAMP. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/97a7cd935f22
Update and sign DAMP add-on. r=ochameau

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ae2699e5b050
https://hg.mozilla.org/mozilla-central/rev/97a7cd935f22