Closed Bug 1882964 Opened 4 months ago Closed 2 months ago

Make style-utils.js getRuleText faster

Categories

(DevTools :: Inspector, task)

task

Tracking

(firefox125 wontfix, firefox126 fixed)

RESOLVED FIXED
125 Branch
Tracking Status
firefox125 --- wontfix
firefox126 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

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),
  };
}
Assignee: nobody → nchevobbe
Pushed by nchevobbe@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 125 Branch

== 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

Regressions: 1884230

Bug 1884230 reverted this change

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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.

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.

Attachment #9391395 - Attachment description: Bug 1882964 - [devtools] Add InspectorUtils.replaceRuleBodyText. r=#layout. → Bug 1882964 - [devtools] Add InspectorUtils.replaceRuleBodyText. r=#layout,#devtools-reviewers.
Attachment #9391396 - Attachment description: Bug 1882964 - [devtools] Turn getRuleBodyTextOffsets into getRuleBodyText for easier unicode chars handling. r=#layout. → Bug 1882964 - [devtools] Turn getRuleBodyTextOffsets into getRuleBodyText for easier unicode chars handling. r=#layout,#devtools-reviewers.
Blocks: 1606641
Attachment #9391395 - Attachment description: Bug 1882964 - [devtools] Add InspectorUtils.replaceRuleBodyText. r=#layout,#devtools-reviewers. → Bug 1882964 - [devtools] Add InspectorUtils.replaceBlockRuleBodyTextInStylesheet. r=#layout,#devtools-reviewers.
Attachment #9388587 - Attachment is obsolete: true
Attachment #9388588 - Attachment is obsolete: true
Pushed by nchevobbe@mozilla.com:
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.
Status: REOPENED → RESOLVED
Closed: 3 months ago2 months ago
Resolution: --- → FIXED
Regressions: 1890775

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.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: