Closed Bug 1415532 Opened 7 years ago Closed 7 years ago

Run DAMP open/reload/close tests against a document specific to each tool

Categories

(DevTools :: General, defect, P2)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Bug 1415532 - Convert damp.js from Task.jsm to async/await.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1415532 - Run DAMP test against a document specific to the inspector.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
DAMP currently run generci test got all panels.
It records:
* toolbox opening
* page reload, or panel update during page reload
* toolbox close

That, for each panel and against two documents:
* "simple": a naive webpage with nothing in it.
* "complicated": a local copy of old version of bild.de website.

"simple" help identifying panel startup regression.
But "complicated" doesn't help identifying various regressions than depends on the content or behavior of the webpage.

We should introduce new documents, this time specific to each tool, to better cover all scenarios.
Priority: -- → P2
Assignee: nobody → poirot.alex
I only really looked into the first patch for the inspector, the others is just to show how it would replicate to all tools.

PerfHerder is broken this morning and doesn't display test results anymore.
Waiting for it to work again before moving forward here...
Depends on: 1418929
Comment on attachment 8930039 [details]
Bug 1415532 - Run DAMP test against a document specific to the inspector.

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f675a717a8ca02918b6d0541d49667a67b9cc9ec&newProject=try&newRevision=a8b73c71ba03ed18b87b1122e9f4ef982b8e42a9&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=custom&framework=1
Using a document with a tree of 1000 DOM Nodes followed by 1000 <div> makes the test too slow to run:
  custom.inspector.open 5,206.31 ± 1.12%
  custom.inspector.reload 6,013.90 ± 3.16%
  custom.inspector.close 158.31 ± 2.96%

I imagine it highlights some particular slowness in the inspector.
We could work on that and make it so that it is much faster,
but we should land the test first and let is run in about or less that one second.
Blocks: 1419326
Blocks: 1419327
Blocks: 1419328
Bug 1188526 highlights a test case that used to be slow in inspector, may be I should also add some dynamically inserted node as well.
Attachment #8930041 - Attachment is obsolete: true
Attachment #8930042 - Attachment is obsolete: true
Attachment #8930038 - Flags: review?(jdescottes)
Comment on attachment 8930039 [details]
Bug 1415532 - Run DAMP test against a document specific to the inspector.

With just this patch I got this:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=95bc1a053f50fc2e8a7f2d2f53d99744cca1c2ab&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=custom.ins&framework=1&selectedTimeRange=172800
  custom.inspector.open  2,300.08 ± 3.40%
  custom.inspector.reload  1,806.97 ± 0.54% 
  custom.inspector.close  92.91 ± 4.95%
And the noise of DAMP goes from 37.26 to 52.46 = +40.82%
Comment on attachment 8930040 [details]
Bug 1415532 - Add element asynchronously.

And with this additional patch, which also adds elements asynchronously, I get this result:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=17ca512e8106249c83d7f0e450ef3486f8a46de9&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=custom.ins&framework=1&selectedTimeRange=172800
  custom.inspector.open  2,955.58 ± 2.51%
  custom.inspector.reload  2,523.96 ± 0.36%
  custom.inspector.close  102.10 ± 2.07%
And DAMP noise goes from 35.00 to 37.14 = +6.12% 

I'm not completely confident about this.
May be I should work more on that in a followup.
This async element adding could be in a distinct step, after reload is over? I'm not sure about that. May be it is still worth seeing the impact of it *during* open and reload? It is hard to make the call.

This patch related to existing inspector.mutation test. The main difference is that it will execute it against a more complex document, which I think, is a good thing as it may highlight perf issues involving multiple factors.
There is also an implementation detail. It is slightly more high level as it check for the last element added to appear in the markup view rather than relying on markupmutation event counts.
Comment on attachment 8930038 [details]
Bug 1415532 - Convert damp.js from Task.jsm to async/await.

https://reviewboard.mozilla.org/r/201230/#review207478

Thanks for the cleanup!
Attachment #8930038 - Flags: review?(jdescottes) → review+
Comment on attachment 8930039 [details]
Bug 1415532 - Run DAMP test against a document specific to the inspector.

https://reviewboard.mozilla.org/r/201232/#review207482

I had a comment about adding the script used to generate the markup, but it's in the next changeset.
Would be nice to have it in this changeset, but not a big deal.
Attachment #8930039 - Flags: review?(jdescottes) → review+
Comment on attachment 8930040 [details]
Bug 1415532 - Add element asynchronously.

https://reviewboard.mozilla.org/r/201234/#review207484

This is mostly r+ but since I had a bunch of comments, I can at least take another look :)

Re: your point about the usefulness of the async elements. Hard to say. 

By design, with a setInterval(, 50) running 10 times, we are adding 500ms to the test which have nothing to do with devtools performance.
That's between 17% and 20% of the total time for the open and reload tests. 
Open was 650ms slower with the async, reload was 700ms slower. So right now, most of the impact caused by this async step comes from the
setInterval waiting for the next iteration.

Should we measure it separately? I think the purpose of this test is to have a heavy/complex page to measure tool open/close/reload. 
If we start having custom sequential steps in the test, I fear it's going to be a pain to maintain.

How about: keep the current approach and try to reduce the setInterval delay. See if we can get something where devtools performance
play a bigger role.

::: commit-message-46f0f:1
(Diff revision 2)
> +Bug 1415532 - Add element asynchronously. r=jdescottes

let's mention DAMP or Talos in this commit message

::: testing/talos/talos/tests/devtools/addon/content/damp.js:459
(Diff revision 2)
>      await this.openToolboxAndLog("cold.inspector", "inspector");
>      await this.closeToolbox();
>      await this.testTeardown();
>    },
>  
> -  async reloadInspectorAndLog(label, toolbox) {
> +  async reloadInspectorAndLog(label, toolbox, waitForElement) {

Needs JS doc because the new argument is optional.

::: testing/talos/talos/tests/devtools/addon/content/damp.js:470
(Diff revision 2)
> +      let { walker, markup } = inspector;
> +      let nodeFront;
> +      while (true) {
> +        if (!nodeFront) {
> +          nodeFront = await walker.querySelector(walker.rootNode, waitForElement);
> +        }
> +        let container = markup.getContainer(nodeFront);
> +        if (container) {
> +          break;
> +        }
> +        await inspector.once("markupmutation");

Let's have a shared helper 

 async waitForElement(inspector, selector) {}

this way we can also rewrite the if above as

  if (waitForElementSelector) {
    await this.waitForElement(waitForElementSelector);
  }
  
which is less confusing than the early return.

::: testing/talos/talos/tests/devtools/addon/content/damp.js:489
(Diff revision 2)
>    },
>  
>    async customInspector() {
>      let url = CUSTOM_URL.replace(/\$TOOL/, "inspector");
>      await this.testSetup(url);
> -    let toolbox = await this.openToolboxAndLog("custom.inspector", "inspector");
> +    let onLoad = async function (toolbox, inspector) {

eslint failure here (space after "function")

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:1
(Diff revision 2)
> +// nodejs script to generate: testing/talos/talos/tests/devtools/addon/content/pages/custom/inspector.html

Thanks for including the script! Could be moved to the previous changeset, but not a big deal.

License header missing.

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:11
(Diff revision 2)
> +let deep = 50;
> +// Then we create ${n} element after the deep tree
> +let n = 50;
> +// Number of attributes set on the repeated elements
> +let attributes = 50;
> +// We also add elements asynchronously after the ${n}-th.

Comment explaining why it is interesting to add elements asynchronously?

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:17
(Diff revision 2)
> +let async = 10;
> +
> +// Build the <div> with $attributes data attributes
> +let div = "<div";
> +for(var i = 1; i <= attributes; i++) {
> +  div += " data-a" + i + "=\"" + i + "\"";

nit: more readable as ` data-a${i}="${i}"`

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:24
(Diff revision 2)
> +div += ">";
> +
> +// Build the tree of $deep elements
> +let tree = "";
> +for(var i = 1; i <= deep; i++) {
> +  for(var j = 0; j < i; j++) {

tree += new Array(i).join("&nbsp;");

(would not usually suggest that but {for} loop in a {for} loop makes this part looks more complex than it is IMO)

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:25
(Diff revision 2)
> +
> +// Build the tree of $deep elements
> +let tree = "";
> +for(var i = 1; i <= deep; i++) {
> +  for(var j = 0; j < i; j++) {
> +    tree += "&nbsp;";

Why nbsp here?

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:28
(Diff revision 2)
> +for(var i = 1; i <= deep; i++) {
> +  for(var j = 0; j < i; j++) {
> +    tree += "&nbsp;";
> +  }
> +  if (i == deep) {
> +    tree += div.replace("div", "div id=\"deep\"");

do we need this "deep" id?

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:34
(Diff revision 2)
> +  } else {
> +    tree += div;
> +  }
> +  tree += " " + i + "\n";
> +}
> +for(var i = deep; i >= 0; i--) {

This currently closes one extra div. 
i >= 1

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:35
(Diff revision 2)
> +  for(var j = 0; j < i; j++) {
> +    tree += " ";
> +  }

same suggestion: tree += new Array(i).join(" ");

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:48
(Diff revision 2)
> +for(var i = 1; i <= n; i++) {
> +  repeat += div + " " + i + " </div>\n";
> +}
> +
> +console.log(`
> +<!DOCTYPE html>

Should probably output a license header too ?

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:49
(Diff revision 2)
> +  repeat += div + " " + i + " </div>\n";
> +}
> +
> +console.log(`
> +<!DOCTYPE html>
> +<html>

Could be nice to also output a warning:

"This file is a generated file, do not edit it directly. See testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js for instructions to update this file" or something like it.

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:61
(Diff revision 2)
> +  }
> +  </style>
> +</head>
> +<body>
> +<script>
> +  // Add element asynchronously

nit: element -> elements

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:67
(Diff revision 2)
> +  let x = 0;
> +  let count = ${async};
> +  let interval = window.setInterval(() => {
> +    let div = document.createElement("div");
> +    document.body.appendChild(div);
> +    document.body.setAttribute("increment", x++);

Maybe add a comment here: why are we setting this attribute? is it to perform additional mutations and stress the inspector or for test purposes.

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:76
(Diff revision 2)
> +      // Stop adding elements
> +      window.clearInterval(interval);
> +    }
> +  }, 50);
> +</script>
> +<!-- <div> elements with ${deep} nested childs, all with ${attributes} attributes -->

childs -> children
Attachment #8930040 - Flags: review?(jdescottes)
Attachment #8930040 - Attachment is obsolete: true
Comment on attachment 8930039 [details]
Bug 1415532 - Run DAMP test against a document specific to the inspector.

I merged the node script into this changeset and tried to address all comments about this first patch.

I'll keep the async node creation for a followup as it is more challenging.
Having it part of the document load is really challenging as doing setInterval(-,0) seems to be invisible. In the markup view I only see the very last value (as if things were synchronous). I'll look more seriously into that in the followup.
Attachment #8930039 - Flags: review+ → review?(jdescottes)
Comment on attachment 8930039 [details]
Bug 1415532 - Run DAMP test against a document specific to the inspector.

https://reviewboard.mozilla.org/r/201232/#review207842

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:33
(Diff revision 3)
> +  for(var j = 0; j < i; j++) {
> +    tree += " ";
> +  }

still my nit about new Array(i).join :) Since you did it in the first spot might as well do it here too.
Attachment #8930039 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes][:julian] from comment #16)
> :::
> testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-
> inspector-html.js:25
> (Diff revision 2)
> > +
> > +// Build the tree of $deep elements
> > +let tree = "";
> > +for(var i = 1; i <= deep; i++) {
> > +  for(var j = 0; j < i; j++) {
> > +    tree += "&nbsp;";
> 
> Why nbsp here?

I tried to do some margin so that divs look like a tree in the page,
but ended up using a css, it was much easier.
=> Replaced that by a space.

> :::
> testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-
> inspector-html.js:28
> (Diff revision 2)
> > +for(var i = 1; i <= deep; i++) {
> > +  for(var j = 0; j < i; j++) {
> > +    tree += "&nbsp;";
> > +  }
> > +  if (i == deep) {
> > +    tree += div.replace("div", "div id=\"deep\"");
> 
> do we need this "deep" id?

No, I was thinking about the future, where we would select a deep node.
Got rid of that until we use it.

> testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-
> inspector-html.js:33
> (Diff revision 3)
> > +  for(var j = 0; j < i; j++) {
> > +    tree += " ";
> > +  }
> 
> still my nit about new Array(i).join :) Since you did it in the first spot
> might as well do it here too.

Right, I missed it. It also fixed a bug with these spaces...
For the record, I tried various settings to better understand this test.

# Landed patch with everything:
# 50 attributes, 50 deep, 50 repetitions.
open 2,434.36 ± 4.12%
reload 1,489.75 ± 2.46%

# Only one attribute, 50 deep, 50 repetitions.
open 1,234.74 ± 7.30% 
reload 469.21 ± 0.93% 

# 50 attributes, 0 deep, 50 repetitions.
open 2,417.66 ± 3.38% 
reload 1,448.91 ± 0.69% 

# 50 attributes, 50 deep, 0 repetitions.
open 1,037.48 ± 9.16%
reload 302.75 ± 1.11%

# To compare, with simple document:
open 865.95 ± 0.91% 
reload 253.90 ± 2.85%

=> Both tests are mostly impacted by having a lots of elements directly under <body> element (which makes sense as we expand it) while nested children do not have much impact. Attributes just seems to "pile up" on the number of elements.

My plan is to followup on this test by adding more subtests.
I think we could track the time it takes to expand the tree by selecting the bottom most children.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18ecb8448c59
Convert damp.js from Task.jsm to async/await. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/f15c8bf67a90
Run DAMP test against a document specific to the inspector. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/18ecb8448c59
https://hg.mozilla.org/mozilla-central/rev/f15c8bf67a90
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Depends on: 1420364
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: