1473332 - Add DAMP test for Autocomplete popup

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

The test should measure how much time it takes to open, filter and close the popup

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1473332 - Add a DAMP test for console autocomplete; .

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

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8990665 [details]
Bug 1473332 - Add a DAMP test for console autocomplete; .

https://reviewboard.mozilla.org/r/255748/#review262474

It looks good to me, but I would like to see if you could lower the typical variance of this test.
As-is, it will harly catch regressions under 3%.
The best is to try various settings and push to try.

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/autocomplete.js:20
(Diff revision 1)
> +const TEST_NAME = "console.autocomplete";
> +
> +
> +module.exports = async function() {
> +  await testSetup(`data:text/html,<meta charset=utf8><script>
> +    const items = Array.from({length: 100}).reduce((res, _, i) => ({

Did you tried other number other than 100?
And/or tried doing more than one autocomplete?

As-is the test is having a noise of 2.9%, while I tried to make it so that most of the tests (especialy the open and reload ones) are under 1%:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=2f89910ad07652e1f99556558c078dd788d1cae1&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=auto&framework=1&selectedTimeRange=172800

Tweaking the test to be more intense sometimes lower its noise. Otherwise running in more than once helps.

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/autocomplete.js:23
(Diff revision 1)
> +module.exports = async function() {
> +  await testSetup(`data:text/html,<meta charset=utf8><script>
> +    const items = Array.from({length: 100}).reduce((res, _, i) => ({
> +      ...res,
> +      ["item" + i.toString().padStart(2, "0")]: i
> +    }), {});

In term of performance, this code is very bad.
It will:
* call a method 100 times whereas we could use a loop
* create an object 100 times while duplicating the previous object that is big and bigger 100 times!!

Not whithout saying it is criptic when you don't know these two "advanced" methods: Array.from and reduce.

i.e. please use a loop here and look more like C code :)

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/autocomplete.js:48
(Diff revision 1)
> +  // order to display the popup.
> +  jsterm.complete(jsterm.COMPLETE_HINT_ONLY);
> +  await onPopUpOpened;
> +  test.done();
> +
> +  await closeToolboxAndLog(TEST_NAME, toolbox);

Is there really a value in tracking close?
It doesn't looks like a typical regression to watch.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8990665 [details]
Bug 1473332 - Add a DAMP test for console autocomplete; .

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

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8990665 [details]
Bug 1473332 - Add a DAMP test for console autocomplete; .

https://reviewboard.mozilla.org/r/255748/#review265756

Thanks Nicolas, it looks good.

Do you have a link to DAMP for this patch?

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/autocomplete.js:45
(Diff revisions 1 - 2)
> +    await showAndHideAutoCompletePopup(jsterm);
> +  }
> +
> +  test.done();
> +
> +  await closeToolbox(TEST_NAME, toolbox);

closeToolbox expects no arguments

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Here's a try push with the updated test https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=a96e52eafe34bbb8fd25d90329f6fea9a95fc0f1&framework=1&filter=damp&selectedTimeRange=172800

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
> Here's a try push with the updated test
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> central&newProject=try&newRevision=a96e52eafe34bbb8fd25d90329f6fea9a95fc0f1&f
> ramework=1&filter=damp&selectedTimeRange=172800

1.26%, it is pretty good noise, thanks for the improvement!

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8990665 [details]
Bug 1473332 - Add a DAMP test for console autocomplete; .

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5865394c3ef1
Add a DAMP test for console autocomplete; r=ochameau.

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/5865394c3ef1