Closed Bug 1432416 Opened 6 years ago Closed 6 years ago

Test selecting an element with lot of CSS rules in DAMP

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: ochameau, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1432416 - Use DAMP test helper for inspector.layout.open;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1432416 - Add talos/DAMP test for inspector with many CSS rules;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1432416 - Add comments and documentation about adding new DAMP tests;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1432416 - Consistent folder hierarchy for DAMP custom test pages;
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
Bug 1415940 landed without triggering any improvement to DAMP.
It looks like we are missing a test in DAMP selecting a node with a lot of CSS rules. custom page has elements with lots of attributes, but not CSS rules, we should also apply rules to custom elements.
Priority: -- → P2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Added new test that selects an element with many rules, pushed to talos:
- baseline + test
- baseline + test + revert Bug 1415940

to check that the improvement is visible.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=cde3b8ab829ebecd54c32f1689b056ebb4a1f4f9&newProject=try&newRevision=51df2414c69882c1948e0dd747a47761be028596&framework=1&filter=damp
My new test seems to have a big impact on subsequent simple inspector test: 

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=18fdbdef748d&newProject=try&newRevision=51df2414c69882c1948e0dd747a47761be028596&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1

This comparison is : my baseline vs my baseline + new test, so normally there shouldn't be any external change polluting the results. 

So far I don't understand why. The closeToolbox+testTeardown should be enough to wait for pending requests to settle and to force GC.
You wrote the test the old way. Now, there is a test function helper documented here:
  http://docs.firefox-dev.tools/tests/writing-perf-tests.html#how-to-write-a-performance-test
This helper automatically adds markers.

Otherwise, I had in mind to contribute to custom page instead of adding yet another test tab.
Adding all scenarios to custom page surely mixes the cases, but it should also help
identifying slow scenarios involving complex setup.
I'm afraid we will never get a very good coverage with a combination of naive test pages.
Note that you can still have an independant test. My comment here is only about having an independant test document.

(In reply to Julian Descottes [:jdescottes][:julian] from comment #2)
> So far I don't understand why. The closeToolbox+testTeardown should be
> enough to wait for pending requests to settle and to force GC.

With markers (or just looking at the logs), you can see that your new test is ran at the very end.
There is only one ran after it, panelsInBackground.reload.
But on automation, we run all the tests 5 times before restarting firefox.
So after panelsInBackground.reload, simple.webconsole and simple.inspector are going to follow.

At the end you are making console opening slightly faster to open while inspector is slightly slower to open and reload. (I would totally ignore all close tests for now)

Unfortunately, this is pretty common behavior. You often slightly change the timings of following tests when working on a new one. I'm wondering if that's something at OS level, where allocating a lot of memory will later slow down your process.
If you want to dig that, I would suggest adding a long pause (setTimeout) at the end of your test,
just to confirm that there is really no pending operations. You may also try to do even more GCs, at the end of your test to see if that's about GC which isn't complete.
Thanks for having a look.

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> You wrote the test the old way. Now, there is a test function helper
> documented here:
>  
> http://docs.firefox-dev.tools/tests/writing-perf-tests.html#how-to-write-a-
> performance-test
> This helper automatically adds markers.
> 
> Otherwise, I had in mind to contribute to custom page instead of adding yet
> another test tab.
> Adding all scenarios to custom page surely mixes the cases, but it should
> also help
> identifying slow scenarios involving complex setup.
> I'm afraid we will never get a very good coverage with a combination of
> naive test pages.
> Note that you can still have an independant test. My comment here is only
> about having an independant test document.
> 

I can change that even though I favor isolated test documents. Having a single test document is going to make tests harder to write/maintain and will increase side effects between tests. 

> (In reply to Julian Descottes [:jdescottes][:julian] from comment #2)
> > So far I don't understand why. The closeToolbox+testTeardown should be
> > enough to wait for pending requests to settle and to force GC.
> 
> With markers (or just looking at the logs), you can see that your new test
> is ran at the very end.
> There is only one ran after it, panelsInBackground.reload.
> But on automation, we run all the tests 5 times before restarting firefox.
> So after panelsInBackground.reload, simple.webconsole and simple.inspector
> are going to follow.
> 
> At the end you are making console opening slightly faster to open while
> inspector is slightly slower to open and reload. (I would totally ignore all
> close tests for now)
> 
> Unfortunately, this is pretty common behavior. You often slightly change the
> timings of following tests when working on a new one. I'm wondering if
> that's something at OS level, where allocating a lot of memory will later
> slow down your process.
> If you want to dig that, I would suggest adding a long pause (setTimeout) at
> the end of your test,
> just to confirm that there is really no pending operations. You may also try
> to do even more GCs, at the end of your test to see if that's about GC which
> isn't complete.

I expect slight impacts when adding another test but here having more than 30% of slowdown is quite big.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #4)
> I can change that even though I favor isolated test documents. Having a
> single test document is going to make tests harder to write/maintain

That's a good point. I do share this concern.

> and will increase side effects between tests. 

But that's exactly what I'm looking for. Actually that's not really side effects
that I'm looking for, but more about crossing the cases and codepaths.

I think that if we do only unit tests, we would have to write tons of them to have a wide coverage.
For example, here about rules, by contributing to custom page,
we will also cover the cost of having a document with many rules when opening the inspector.
Will it slow down the inspector for some reason? Here we will know as it will be part of custom.inspector.open.

Also, I had in mind to execute this test during customInspector() run, after reload.
That would prevent having to open/close a tab and speed up DAMP execution.

Does that make sense overall? I could be convinced that I'm not heading in the right direction...

> I expect slight impacts when adding another test but here having more than
> 30% of slowdown is quite big.

Percentages have to be put in perspective of the original durations.
inspector.close, the 35% one is only 10ms slower. Again, I still consider close tests as flaky.
The codepath to close a toolbox is full of mystery
(In reply to Alexandre Poirot [:ochameau] from comment #8)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #4)
> > I can change that even though I favor isolated test documents. Having a
> > single test document is going to make tests harder to write/maintain
> 
> That's a good point. I do share this concern.
> 
> > and will increase side effects between tests. 
> 
> But that's exactly what I'm looking for. Actually that's not really side
> effects
> that I'm looking for, but more about crossing the cases and codepaths.
> 

I'm worried this makes regression investigation more complex. eg: We have a lot of attributes on this test page. If a change regresses attributes handling, I would prefer that only the tests checking attributes show a significant change.

> I think that if we do only unit tests, we would have to write tons of them
> to have a wide coverage.
> For example, here about rules, by contributing to custom page,
> we will also cover the cost of having a document with many rules when
> opening the inspector.
> Will it slow down the inspector for some reason? Here we will know as it
> will be part of custom.inspector.open.
> 
> Also, I had in mind to execute this test during customInspector() run, after
> reload.
> That would prevent having to open/close a tab and speed up DAMP execution.
> 

I agree keeping DAMP's runtime "low" is important, it's already slow enough. If that's ok, I will add comments in damp.js to guard people from adding new individual tests in this case. eg. "Avoid adding new tests here, consider contributing to the custom test page for your tool instead.". If needed we could add a link to more documentation which explains why we need to group asserts in a single test. Will ease contributing to damp, at least for me :)

> Does that make sense overall? I could be convinced that I'm not heading in
> the right direction...
> 

It makes sense, it's not really a straightforward question. Both approaches have their merits, it's exactly like comparing integration tests and unit tests. One will catch more issues but will be harder to maintain and investigate (and vice versa). We need to balance both. I usually prefer to start with unit tests and think about what makes sense to test together in a bigger test in a second step. For DAMP though, keeping the overall runtime of the suite low is a good argument to try and push for less isolated tests.

> > I expect slight impacts when adding another test but here having more than
> > 30% of slowdown is quite big.
> 
> Percentages have to be put in perspective of the original durations.
> inspector.close, the 35% one is only 10ms slower. Again, I still consider
> close tests as flaky.
> The codepath to close a toolbox is full of mystery

Fair enough, I assumed "close" tests were reliable, while I knew settle et al. were not for instance. I'll ignore those for now.
Talos comparison: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=a4de8b2adb16&newProject=try&newRevision=e43fbca884218e8b1745edc86f9cc74a96bb4a10&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=1

New tests:
- custom.inspector.manyrules.selectnode : 479ms
- custom.inspector.manyrules.deselectnode : 94ms
Significant changes:
- custom.inspector.close 50% slower

Interestingly, earlier today I was running the manyrules.* tests before the reload. It made the reload test faster, but it did not impact the close.
Comment on attachment 8952329 [details]
Bug 1432416 - Use DAMP test helper for inspector.layout.open;

https://reviewboard.mozilla.org/r/221354/#review229084

Thanks for the "maintenance" :)
Attachment #8952329 - Flags: review?(poirot.alex) → review+
Comment on attachment 8952330 [details]
Bug 1432416 - Add talos/DAMP test for inspector with many CSS rules;

https://reviewboard.mozilla.org/r/221582/#review229066

Looks good, could be slightly improved to cover even more cases.

Note that, via the profiler I can see a small leftover after deselect:

A full profile of:
./mach talos-test --activeTests damp --geckoProfile --cycles 1 --tppagecycles 1 --geckoProfileEntries 10000000 --subtest custom.inspector
https://perfht.ml/2EXrAam
  
And a view zoomed at what happens between deselect and close test:
https://perfht.ml/2FwBUHB
You can see that there is a call to selectElement/showNode, followed by a reflow.
And at the very end of it, there is also a call to `rules.js:_populate`.

::: testing/talos/talos/tests/devtools/addon/content/pages/custom/generate-inspector-html.js:47
(Diff revision 4)
>  
> +// Prepare CSS rules to add to the document <style>.
> +let CSS_RULES_COUNT = 200;
> +let manyCssRules = "";
> +for (i = 0; i < CSS_RULES_COUNT; i++) {
> +  manyCssRules += `  .many-css-rules { font-size: ${i}px; }\n`;

It would be slightly more realistic with multiple css attributes being set for each rule.
We end up adding a lot of attributes as there is a lot of rules, but there may be hidden complexity when the number of attributes of one rule grows...
Also, adding special rules, like colors to cover the color picker code would be great. Font family, margin are also special.

Again, this goes against the unit test vision...
feel free to ignore this.
Attachment #8952330 - Flags: review?(poirot.alex) → review+
Comment on attachment 8953017 [details]
Bug 1432416 - Add comments and documentation about adding new DAMP tests;

https://reviewboard.mozilla.org/r/222268/#review229090

Thanks a ton for the documentation.
I should probably revamp that page once all tools have a custom page.
(there is only the netmonitor left, for the important tools, which should be ready soon)
Attachment #8953017 - Flags: review?(poirot.alex) → review+
Comment on attachment 8953018 [details]
Bug 1432416 - Consistent folder hierarchy for DAMP custom test pages;

https://reviewboard.mozilla.org/r/222270/#review229092
Attachment #8953018 - Flags: review?(poirot.alex) → review+
Thanks for the reviews! Added some more rules but voluntarily avoided anything that triggers swatch editors (for instance colors).

> Note that, via the profiler I can see a small leftover after deselect:

I think that's expected. First I select a node that should have no rules, and measure the deselect time. Then I select back the initial node:
  await selectNodeFront(initialNodeFront);

My reasoning was that since I don't control what is or what applies to this initial node, I should not use it for the "deselect" test. But I still reselect it at the end to restore the initial state after my test.
Results look okay with more rules, landing.

Heads-up for performance sheriffs: we expect DAMP tests to show a regression after landing this, since we are adding a new test.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10334968ce12
Use DAMP test helper for inspector.layout.open;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/27d8719dc8c2
Add talos/DAMP test for inspector with many CSS rules;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/7f42c0c27b28
Add comments and documentation about adding new DAMP tests;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c1d4e29c9e3b
Consistent folder hierarchy for DAMP custom test pages;r=ochameau
(In reply to Julian Descottes [:jdescottes][:julian] from comment #28)
> Results look okay with more rules, landing.
> 
> Heads-up for performance sheriffs: we expect DAMP tests to show a regression
> after landing this, since we are adding a new test.

Thanks for the notification!
Depends on: 1441538
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: