1979011 - Reduce the number of created elements in TextPropertyEditor

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, task)

Description

[Nicolas Chevobbe [:nchevobbe]]

In TextPropertyEditor, we're creating all the elements that we might need and hide them if we don't: https://searchfox.org/mozilla-central/rev/86878e73a24fe32ea09dbae5b55362efaf7485c8/devtools/client/inspector/rules/views/text-property-editor.js#102,168,211,215,243,247,249,256,258,261,263,266,270,274,287,289,293,295

class TextPropertyEditor {
...
  #create() {
...
    this.expander = createChild(this.container, "button", {
...
    });
...
    this.warning = createChild(this.container, "div", {
...
    });
...
    this.invalidAtComputedValueTimeWarning = createChild(
...
    );
...
    this.unusedState = createChild(this.container, "div", {
...
    });
...
    this.compatibilityState = createChild(this.container, "div", {
...
    });
...
    this.filterProperty = createChild(this.container, "button", {
...
    });
...
    this.computed = createChild(this.element, "ul", {
...
    });
...
    this.shorthandOverridden = createChild(this.element, "ul", {
...
    });

It's quite likely that those different elements won't ever be visible for the majority of declarations, and on elements with lots of declarations, this can impact the time spent in styling and reflow (see https://share.firefox.dev/3GSxfE0 , populating the rules view with a lot of declarations, 30% of the time is spent in Layout)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1979011 - [devtools] Only create elements in TextPropertyEditor once they're needed. r=#devtools.

We used to create elements that we would hide by default, and show them when
we needed them (expander, warning icon, compatibility issue, unusued icon,
invalid at computed value time, computed items, shorthand items).

This increased the time spend in styling for no real benefit, so in this path
we only create those elements once we would display them for the first time.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/52adcb23934c
https://hg.mozilla.org/integration/autoland/rev/1d89cc0cfcef
[devtools] Only create elements in TextPropertyEditor once they're needed. r=devtools-reviewers,jdescottes.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/bd46be085d28
https://hg.mozilla.org/integration/autoland/rev/4eede0427d7c
Revert "Bug 1979011 - [devtools] Only create elements in TextPropertyEditor once they're needed. r=devtools-reviewers,jdescottes." for causing dt failures @ browser_rules_search-filter-computed-list_expander.js

Comment 4

[amarc]

Backed out for causing dt failures

• Backout link
• Push with failures
• Failure Log

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7bb4aec8c72a
https://hg.mozilla.org/integration/autoland/rev/ff53ed6b567f
[devtools] Only create elements in TextPropertyEditor once they're needed. r=devtools-reviewers,jdescottes.

Comment 6

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/ff53ed6b567f

Comment 7

[Florin Bilt]

(In reply to Serban Stanca [:SerbanS] from comment #6)

https://hg.mozilla.org/mozilla-central/rev/ff53ed6b567f

Perfherder has detected a devtools performance change from push ff53ed6b567f30f993c3ed6dc1ee1cb1ca922027.

If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
10%	damp custom.inspector.manycssvariables.selectnode	linux1804-64-shippable-qr	e10s fission stylo webrender	311.19 -> 279.24
10%	damp custom.inspector.manyrules.deselectnode	linux1804-64-shippable-qr	e10s fission stylo webrender	146.56 -> 132.20
10%	damp complicated.jsdebugger.open.DAMP	linux1804-64-shippable-qr	e10s fission stylo webrender	660.91 -> 596.40
6%	damp browser-toolbox.webconsole-ready.DAMP	windows11-64-24h2-shippable	e10s fission stylo webrender	416.36 -> 389.98
5%	damp custom.inspector.manyrules.selectnode	linux1804-64-shippable-qr	e10s fission stylo webrender	822.76 -> 777.66
5%	damp browser-toolbox.webconsole-ready.DAMP	linux1804-64-shippable-qr	e10s fission stylo webrender	670.29 -> 634.03

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 46239

The following documentation link provides more information about this command.