Julian Descottes [:jdescottes] Assignee
	Description • 7 years ago

Bug 1413605 improves significantly the performance of the inspector when the markup view displays a lot of elements. 

We should add a new DAMP test to avoid regressing here.

	Alexandre Poirot [:ochameau]
	Comment 1 • 7 years ago

Expanding elements is one of the important user story in inspector:
https://docs.google.com/document/d/1HfzXs3UwBZrSeUboXFQmFgG9QovgXN1fke4SO7D3m0o/edit#bookmark=id.i0k79epbc7l5

	Julian Descottes [:jdescottes] Assignee
	Comment 2 • 6 years ago

I would like to land this DAMP test before landing the shadow DOM work.

	Julian Descottes [:jdescottes] Assignee
	Comment 4 • 6 years ago

First patch measuring expandAll collapseAll on two nodes with different children distributions.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6340200d20458e4d0953f96f9f83e13add22f2c9&newProject=try&newRevision=7e287c81f63bc5fcf321489a9fa02b5ea752831a&framework=1

	Julian Descottes [:jdescottes] Assignee
	Comment 8 • 6 years ago

talos compare run on win32: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=7afac953cdb3308614d96c99ee8056e38a0bd8d0&newProject=try&newRevision=30927c13dfe28be8d9c1e7a8282d44d125684152&framework=1

	Alexandre Poirot [:ochameau]
	Comment 10 • 6 years ago
mozreview-review

Comment on attachment 8959359 [details]
Bug 1414286 - add DAMP test for inspector expandAll/collapseAll;

https://reviewboard.mozilla.org/r/228192/#review235398

Looks good overall but see my comments.

Linux try results are good, the only unexpected change is +14% on custom.jsdebugger.stepIn.
It would be great to get another try run to see if that is a persistent behavior.

::: testing/talos/talos/tests/devtools/addon/content/tests/inspector/custom.js:19
(Diff revision 4)
>  
>    let toolbox = await openToolboxAndLog("custom.inspector", "inspector");
>    await reloadInspectorAndLog("custom", toolbox);
> +
>    await selectNodeWithManyRulesAndLog(toolbox);
> +  await toolbox.target.client.waitForRequestsToSettle();

`waitForRequestsToSettle` is a stopgap method, it should not be used manually like this.
If there is async requests that you know are still pending, you should make your best to wait for them.
Does it have any effect on DAMP results?

::: testing/talos/talos/tests/devtools/addon/content/tests/inspector/custom.js:23
(Diff revision 4)
>    await selectNodeWithManyRulesAndLog(toolbox);
> +  await toolbox.target.client.waitForRequestsToSettle();
> +  await garbageCollect();
> +
> +  await collapseExpandAllAndLog(toolbox);
> +  await toolbox.target.client.waitForRequestsToSettle();

Same comment about waitForRequestsToSettle.

::: testing/talos/talos/tests/devtools/addon/content/tests/inspector/custom.js:78
(Diff revision 4)
> +  await waitForMultipleChildrenUpdates(inspector);
> +  test.done();
> +
> +  dump("Collapse all children of expand-many-children\n");
> +  test = runTest("custom.inspector.collapseall.manychildren");
> +  inspector.markup.collapseAll(many);

We are only measuring the time it takes to call this method, is that expected? Isn't there any async event to listen to?

::: testing/talos/talos/tests/devtools/addon/content/tests/inspector/custom.js:93
(Diff revision 4)
> +  await waitForMultipleChildrenUpdates(inspector);
> +  test.done();
> +
> +  dump("Collapse all children of expand-balanced\n");
> +  test = runTest("custom.inspector.collapseall.balanced");
> +  inspector.markup.collapseAll(balanced);

Same question here. Any async event to wait for?

	Julian Descottes [:jdescottes] Assignee
	Comment 11 • 6 years ago

Thanks for the review!

(In reply to Alexandre Poirot [:ochameau] from comment #10)
> Comment on attachment 8959359 [details]
> Bug 1414286 - add DAMP test for inspector expandAll/collapseAll;
> 
> https://reviewboard.mozilla.org/r/228192/#review235398
> 
> Looks good overall but see my comments.
> 
> Linux try results are good, the only unexpected change is +14% on
> custom.jsdebugger.stepIn.
> It would be great to get another try run to see if that is a persistent
> behavior.

jsdebugger stepIn used to oscillate between two values a few days ago, but it looks like debugger v24 made the test more stable. In the last windows run, stepin doesn't show up anymore : https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=7afac953cdb3308614d96c99ee8056e38a0bd8d0&newProject=try&newRevision=30927c13dfe28be8d9c1e7a8282d44d125684152&originalSignature=2fa549258445b035f7463afad97852e46b399a6d&newSignature=2fa549258445b035f7463afad97852e46b399a6d&showOnlyConfident=1&framework=1

> 
> :::
> testing/talos/talos/tests/devtools/addon/content/tests/inspector/custom.js:19
> (Diff revision 4)
> >  
> >    let toolbox = await openToolboxAndLog("custom.inspector", "inspector");
> >    await reloadInspectorAndLog("custom", toolbox);
> > +
> >    await selectNodeWithManyRulesAndLog(toolbox);
> > +  await toolbox.target.client.waitForRequestsToSettle();
> 
> `waitForRequestsToSettle` is a stopgap method, it should not be used
> manually like this.
> If there is async requests that you know are still pending, you should make
> your best to wait for them.
> Does it have any effect on DAMP results?
> 
> :::
> testing/talos/talos/tests/devtools/addon/content/tests/inspector/custom.js:23
> (Diff revision 4)
> >    await selectNodeWithManyRulesAndLog(toolbox);
> > +  await toolbox.target.client.waitForRequestsToSettle();
> > +  await garbageCollect();
> > +
> > +  await collapseExpandAllAndLog(toolbox);
> > +  await toolbox.target.client.waitForRequestsToSettle();
> 
> Same comment about waitForRequestsToSettle.
> 

looks like it doesn't make any difference for the impact on custom.inspector close, I will therefore remove it. What do you think about garbageCollect() ? It doesn't seem to change the impact on close either, so I'm tempted to get rid of it as well.

> :::
> testing/talos/talos/tests/devtools/addon/content/tests/inspector/custom.js:78
> (Diff revision 4)
> > +  await waitForMultipleChildrenUpdates(inspector);
> > +  test.done();
> > +
> > +  dump("Collapse all children of expand-many-children\n");
> > +  test = runTest("custom.inspector.collapseall.manychildren");
> > +  inspector.markup.collapseAll(many);
> 
> We are only measuring the time it takes to call this method, is that
> expected? Isn't there any async event to listen to?
> 
> :::
> testing/talos/talos/tests/devtools/addon/content/tests/inspector/custom.js:93
> (Diff revision 4)
> > +  await waitForMultipleChildrenUpdates(inspector);
> > +  test.done();
> > +
> > +  dump("Collapse all children of expand-balanced\n");
> > +  test = runTest("custom.inspector.collapseall.balanced");
> > +  inspector.markup.collapseAll(balanced);
> 
> Same question here. Any async event to wait for?

Collapse does not perform anything async at the moment, it simply updates the UI. I should fire an event from the markup view to make this a bit less implementation specific. I'll add a changeset to do that.

	Patrick Brosset <:pbro>
	Comment 15 • 6 years ago
mozreview-review

Comment on attachment 8961380 [details]
Bug 1414286 - return promise in markup view collapse/expandAll;

https://reviewboard.mozilla.org/r/230144/#review235936

	Julian Descottes [:jdescottes] Assignee
	Comment 16 • 6 years ago

Thanks for the review, let's land this!

	Julian Descottes [:jdescottes] Assignee
	Comment 17 • 6 years ago
mozreview-review-reply

Comment on attachment 8959359 [details]
Bug 1414286 - add DAMP test for inspector expandAll/collapseAll;

https://reviewboard.mozilla.org/r/228192/#review235398

> `waitForRequestsToSettle` is a stopgap method, it should not be used manually like this.
> If there is async requests that you know are still pending, you should make your best to wait for them.
> Does it have any effect on DAMP results?

Dropped it

> We are only measuring the time it takes to call this method, is that expected? Isn't there any async event to listen to?

Made collapseAll and expandAll both return a promise now so we just wait on it in both cases.

	Pulsebot
	Comment 18 • 6 years ago

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d7fa73a97d0
return promise in markup view collapse/expandAll;r=pbro
https://hg.mozilla.org/integration/autoland/rev/6cdea4618a5a
add DAMP test for inspector expandAll/collapseAll;r=ochameau

	Raul Gurzau (:RaulG)
	Comment 19 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7d7fa73a97d0
https://hg.mozilla.org/mozilla-central/rev/6cdea4618a5a