1882964 - Make style-utils.js getRuleText faster

Status: RESOLVED FIXED

(DevTools :: Inspector, task)

Description

[Nicolas Chevobbe [:nchevobbe]]

The function uses the lexer to find the rule "body" text, i.e. the content between opening and closing curly brackets, which can be slow on big stylesheets (see https://bugzilla.mozilla.org/show_bug.cgi?id=1644138#c12).

We can probably move this specific part to InspectorUtils and parse the text in Rust with cssparser which could be faster

---

https://searchfox.org/mozilla-central/rev/6bc0f370cc459bf79e1330ef74b21009a9848c91/devtools/server/actors/utils/style-utils.js#118-183

function getRuleText(initialText, line, column) {
  if (typeof line === "undefined" || typeof column === "undefined") {
    throw new Error("Location information is missing");
  }

  const { offset: textOffset, text } = getTextAtLineColumn(
    initialText,
    line,
    column
  );
  const lexer = getCSSLexer(text);

  // Search forward for the opening brace.
  while (true) {
    const token = lexer.nextToken();
    if (!token) {
      throw new Error("couldn't find start of the rule");
    }
    if (token.tokenType === "symbol" && token.text === "{") {
      break;
    }
  }

  // Now collect text until we see the matching close brace.
  let braceDepth = 1;
  let startOffset, endOffset;
  while (true) {
    const token = lexer.nextToken();
    if (!token) {
      break;
    }
    if (startOffset === undefined) {
      startOffset = token.startOffset;
    }
    if (token.tokenType === "symbol") {
      if (token.text === "{") {
        ++braceDepth;
      } else if (token.text === "}") {
        --braceDepth;
        if (braceDepth == 0) {
          break;
        }
      }
    }
    endOffset = token.endOffset;
  }

  // If the rule was of the form "selector {" with no closing brace
  // and no properties, just return an empty string.
  if (startOffset === undefined) {
    return { offset: 0, text: "" };
  }
  // If the input didn't have any tokens between the braces (e.g.,
  // "div {}"), then the endOffset won't have been set yet; so account
  // for that here.
  if (endOffset === undefined) {
    endOffset = startOffset;
  }

  // Note that this approach will preserve comments, despite the fact
  // that cssTokenizer skips them.
  return {
    offset: textOffset + startOffset,
    text: text.substring(startOffset, endOffset),
  };
}

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1882964 - Add InspectorUtils.getRuleBodyTextOffsets. r=emilio.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1882964 - [devtools] Use `InspectorUtils.getRuleBodyTextOffsets` in `getRuleText`. r=#devtools-reviewers.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6c06c8b6ecd
Add InspectorUtils.getRuleBodyTextOffsets. r=emilio.
https://hg.mozilla.org/integration/autoland/rev/db1073373385
[devtools] Use `InspectorUtils.getRuleBodyTextOffsets` in `getRuleText`. r=devtools-reviewers,ochameau.
https://hg.mozilla.org/integration/autoland/rev/5b0da4101568
apply code formatting via Lando

Comment 4

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/e6c06c8b6ecd
https://hg.mozilla.org/mozilla-central/rev/db1073373385
https://hg.mozilla.org/mozilla-central/rev/5b0da4101568

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

== Change summary for alert #41682 (as of Thu, 07 Mar 2024 04:35:46 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
10%	damp custom.inspector.manycssvariables.selectnode	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	265.57 -> 239.60
8%	damp custom.inspector.manyrules.selectnode	linux1804-64-shippable-qr	e10s fission stylo webrender	572.08 -> 525.49
8%	damp custom.inspector.manyrules.selectnode	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	558.64 -> 513.76
7%	damp browser-toolbox.webconsole-ready.DAMP	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	681.83 -> 633.57
7%	damp custom.inspector.manyrules.selectnode	windows10-64-shippable-qr	e10s fission stylo webrender	494.34 -> 460.01
...	...	...	...	...
5%	damp custom.inspector.manycssvariables.selectnode	windows10-64-shippable-qr	e10s fission stylo webrender	227.19 -> 216.65

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=41682

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Bug 1884230 reverted this change

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1882964 - [devtools] Add InspectorUtils.replaceBlockRuleBodyTextInStylesheet. r=#layout,#devtools-reviewers.

The next patch modifies getRuleText so it only returns the text, and no
longer the offset at which the rule starts.
The only consumre of the returned offset was in StyleRuleActor#setRuleText,
so we migrate this directly to a InspectorUtils method to avoid mixing JS string
indeces with Rust bytes position.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1882964 - [devtools] Turn getRuleBodyTextOffsets into getRuleBodyText for easier unicode chars handling. r=#layout,#devtools-reviewers.

InspectorUtils.getRuleBodyTextOffset was returning bytes position, and we
were using them directly in Javascript substring, which causes problem
with non-ascii chars.
Instead of returning offsets to compute the rule string, we directly return
the string from InspectorUtils which is easier to work with.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c374d883ed01
[devtools] Add InspectorUtils.replaceBlockRuleBodyTextInStylesheet. r=layout-reviewers,devtools-reviewers,emilio,ochameau.
https://hg.mozilla.org/integration/autoland/rev/f6ffb60aead6
[devtools] Turn getRuleBodyTextOffsets into getRuleBodyText for easier unicode chars handling. r=layout-reviewers,devtools-reviewers,emilio,ochameau.

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/c374d883ed01
https://hg.mozilla.org/mozilla-central/rev/f6ffb60aead6

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Perfherder has detected a devtools performance change from push 1b56c653a5ee3a14ec0955b092498999e9333bfa.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
9%	damp custom.inspector.manyrules.selectnode	linux1804-64-shippable-qr	e10s fission stylo webrender	763.88 -> 691.86
8%	damp custom.inspector.manyrules.selectnode	windows10-64-shippable-qr	e10s fission stylo webrender-sw	639.42 -> 585.51
8%	damp custom.inspector.manyrules.selectnode	windows10-64-shippable-qr	e10s fission stylo webrender	637.11 -> 586.82

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 sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 42216

For more information on performance sheriffing please see our FAQ.