1473322 - Stop recording settle subtests in DAMP

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

"settle" subtests on DAMP have been more confusing than helpful so far.
It would be great to keep recording this data over time, but for now it is a burden for every developer facing a regression on it.
Examples are bug 1468150 and bug 1454948.

Comment 1

[Alexandre Poirot [:ochameau]]

Remove it doesn't change the baseline of all the other tests:
  https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=413b493db32c282c87c143fbb5a5430c5aba0b3c&newProject=try&newRevision=52564e4f3471731aa1d5a0ba887f2c8d113ac093&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyComparable=1&framework=1
But changes the "summary" value as all these tests are no longer collected:
  https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=413b493db32c282c87c143fbb5a5430c5aba0b3c&newProject=try&newRevision=52564e4f3471731aa1d5a0ba887f2c8d113ac093&framework=1

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473322 - Stop recording "settle" tests on DAMP.

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

Comment 3

[Yulia Startsev [:yulia]]

Comment on attachment 8990969 [details]
Bug 1473322 - Stop recording "settle" tests on DAMP.

https://reviewboard.mozilla.org/r/255966/#review262826

This looks good, I can't see any issues with this. One question I have is, since I found it useful, do we have a plan for informing people writing these damp tests that something is outside of what they are testing?

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Yulia Startsev [:yulia] from comment #3)
> Comment on attachment 8990969 [details]
> Bug 1473322 - Stop recording "settle" tests on DAMP.
> 
> https://reviewboard.mozilla.org/r/255966/#review262826
> 
> This looks good, I can't see any issues with this. One question I have is,
> since I found it useful, do we have a plan for informing people writing
> these damp tests that something is outside of what they are testing?

Not in the current state of things.
I considering two options, but didn't had time to look into any of that:
* have DAMP scripts ran as mochitests and see if we can replace that with assertions.
  Something like, there should be no pending reflows, requests, ...
  But that may replace problematic DAMP regression report into problematic mochitest intermittent.
* have DAMP being run a second time, where all the data is collected and graphed,
  but no reports are being made if anything regress. We could collect various experimental data
  like settle, JS allocations, ...

What I can do in this patch is still print the settle timings in stdout, without recording them, still.

Comment 5

[Yulia Startsev [:yulia]]

That sounds like a good compromise -- that way we could track when we are making changes to the damp tests.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8990969 [details]
Bug 1473322 - Stop recording "settle" tests on DAMP.

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

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8990969 [details]
Bug 1473322 - Stop recording "settle" tests on DAMP.

Reflaging for review as I made significant changes.

Here is the kind of logs this new patch prints on stdout:
08:48:38     INFO -  PID 83471 | Executing test 'debugger/custom.js'
08:48:41     INFO -  PID 83471 | Waiting for state change: count sources
08:48:41     INFO -  PID 83471 | Finished waiting for state change: count sources
08:48:41     INFO -  PID 83471 | Selecting source: App.js
08:48:42     INFO -  PID 83471 | Waiting for state change: selected source
08:48:42     INFO -  PID 83471 | Finished waiting for state change: selected source
08:48:42     INFO -  PID 83471 | Waiting until: text is visible
08:48:42     INFO -  PID 83471 | Finished Waiting until: text is visible
08:48:42     INFO -  PID 83471 | Waiting until: has file metadata
08:48:42     INFO -  PID 83471 | Finished Waiting until: has file metadata
08:48:42     INFO -  PID 83471 | 'custom.jsdebugger.open.settle.DAMP' took 43.398111000000426ms.
08:48:42     INFO -  PID 83471 | Garbage collect
08:48:43     INFO -  PID 83471 | Reload page on 'custom.jsdebugger'
08:48:43     INFO -  PID 83471 | Waiting for state change: count sources
08:48:44     INFO -  PID 83471 | Finished waiting for state change: count sources
08:48:44     INFO -  PID 83471 | Waiting until: text is visible
08:48:45     INFO -  PID 83471 | Finished Waiting until: text is visible
08:48:45     INFO -  PID 83471 | Waiting until: has file metadata
08:48:45     INFO -  PID 83471 | Finished Waiting until: has file metadata
08:48:45     INFO -  PID 83471 | Wait for pending paints on 'custom.jsdebugger'
08:48:45     INFO -  PID 83471 | 'custom.jsdebugger.reload.settle.DAMP' took 24.53803700000026ms.
08:48:45     INFO -  PID 83471 | Waiting for debugger panel

Comment 8

[Alexandre Poirot [:ochameau]]

New comparison:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=413b493db32c282c87c143fbb5a5430c5aba0b3c&newProject=try&newRevision=86089fb8645b29de910072924e28a4433b09fac9&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyComparable=1&framework=1

Comment 9

[Yulia Startsev [:yulia]]

Comment on attachment 8990969 [details]
Bug 1473322 - Stop recording "settle" tests on DAMP.

https://reviewboard.mozilla.org/r/255966/#review263046

I like this solution a lot. We can maybe control this somehow from the command line in the future. I have two tiny nits, but they are not significant at all.

::: testing/talos/talos/tests/devtools/addon/content/tests/head.js:111
(Diff revision 2)
>    let test = runTest(name + ".open.DAMP");
>    let toolbox = await openToolbox(tool, onLoad);
>    test.done();
>  
> -  test = runTest(name + ".open.settle.DAMP");
> +  // Settle test isn't recorded, it only prints the pending duration
> +  test = runTest(name + ".open.settle.DAMP", false);

nit: we could use a template string here 

`${name}.open.settle.damp`

::: testing/talos/talos/tests/devtools/addon/content/tests/head.js:140
(Diff revision 2)
>    await damp.reloadPage(onReload);
>    test.done();
>  
> +  // Settle test isn't recorded, it only prints the pending duration
>    dump("Wait for pending paints on '" + name + "'\n");
> -  test = runTest(name + ".reload.settle.DAMP");
> +  test = runTest(name + ".reload.settle.DAMP", false);

same nit as above, could be a template string

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8990969 [details]
Bug 1473322 - Stop recording "settle" tests on DAMP.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d44bdd3fdd4
Stop recording "settle" tests on DAMP. r=yulia

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Once this lands, do you want to request uplift to beta as well?

Comment 13

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2d44bdd3fdd4