Skip to content

perf(linter/unicorn/prefer-dom-node-text-content): change dispatch to run only on less common node types#23897

Merged
camc314 merged 2 commits into
mainfrom
perf/prefer-dom-node-text-content-dispatch
Jul 2, 2026
Merged

perf(linter/unicorn/prefer-dom-node-text-content): change dispatch to run only on less common node types#23897
camc314 merged 2 commits into
mainfrom
perf/prefer-dom-node-text-content-dispatch

Conversation

@connorshea

@connorshea connorshea commented Jun 28, 2026

Copy link
Copy Markdown
Member

Somewhat similar to #23867 and #23868.

This was generated using Claude Code after I noticed this rule was one of the slowest on the vscode repo. I have reviewed and tested the change by running it on the vscode codebase, and the # of violations before and after is identical. All tests also pass.

I've also had Claude run a battery of tests on code it generated to test as many scenarios as possible, to ensure that there were no differences in output on main vs. this branch, and confirmed that there were indeed no differences.

What changed

The rule's NODE_TYPES were already derived, but two of them — IdentifierName and IdentifierReference — are among the most common node types in any program (every property name, every variable read). The destructuring cases dispatched on those identifiers and then walked up to their parents to detect destructuring patterns.

This PR instead dispatches directly on the rare parent nodes and reads the key/binding:

Old dispatch (common) New dispatch (rare) Covers
IdentifierName BindingProperty (key) + AssignmentTargetPropertyProperty (name) const {innerText} = node, ({innerText: text} = node)
IdentifierReference AssignmentTargetPropertyIdentifier (binding) ({innerText} = node)

StaticMemberExpression (node.innerText) is unchanged.

Why it's behavior-identical

  • The only IdentifierName that can be a direct child of BindingProperty/AssignmentTargetPropertyProperty is the key (values bottom out in BindingIdentifier/IdentifierReference). The old grandparent set always matched (ObjectPattern/ObjectAssignmentTarget), so that branch always fired — matching the new unconditional trigger. Matching only PropertyKey::StaticIdentifier preserves the computed-key exclusion ({[innerText]: text}).
  • The AssignmentTargetPropertyIdentifier arm keeps the same parent-of-ObjectAssignmentTarget check. The old non-APTI path was dead: a rest target (({...innerText} = node)) goes through AssignmentTargetRest, never a direct ObjectAssignmentTarget child.

The rule test snapshot is byte-identical (no .snap change).

Results

Running oxlint --debug=timings --quiet -A all -W unicorn/prefer-dom-node-text-content on the vscode repo.

Before:

Found 245 warnings and 47 errors.

Rule                                   Time (ms)  Relative    Calls  Source
------------------------------------  ----------  --------  -------  ------
unicorn/prefer-dom-node-text-content     116.624    100.0%  5996077  native

After:

Found 245 warnings and 47 errors.

Rule                                   Time (ms)  Relative    Calls  Source
------------------------------------  ----------  --------  -------  ------
unicorn/prefer-dom-node-text-content      23.631    100.0%  1437765  native

Violations are unchanged (245 warnings, 47 errors). Calls drop 76% (5,996,077 → 1,437,765) and rule time drops ~80% (116ms → 23ms).

🤖 Generated with Claude Code

@github-actions github-actions Bot added the A-linter Area - Linter label Jun 28, 2026
@connorshea
connorshea force-pushed the perf/prefer-dom-node-text-content-dispatch branch from d51dc63 to e30436a Compare June 28, 2026 14:48
@connorshea
connorshea marked this pull request as ready for review June 28, 2026 14:49
@connorshea
connorshea requested a review from camc314 as a code owner June 28, 2026 14:49
@connorshea connorshea changed the title perf(linter/unicorn/prefer-dom-node-text-content): only dispatch for relevant node types perf(linter/unicorn/prefer-dom-node-text-content): change dispatch to run only on less common node types Jun 28, 2026
@codspeed-hq

codspeed-hq Bot commented Jun 28, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 5 untouched benchmarks
⏩ 66 skipped benchmarks1


Comparing perf/prefer-dom-node-text-content-dispatch (a2f43ff) with main (fdbd34d)

Open in CodSpeed

Footnotes

  1. 66 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@camc314 camc314 self-assigned this Jun 28, 2026
…cturing nodes instead of identifiers

The rule's destructuring cases dispatched on `IdentifierName` and
`IdentifierReference` — two of the most common node types (every property
name and variable read) — then walked up to their parents to detect
destructuring patterns. Instead, dispatch directly on the rare parent
nodes (`BindingProperty`, `AssignmentTargetPropertyProperty`,
`AssignmentTargetPropertyIdentifier`) and read the key/binding.

Behavior is identical:
- The only `IdentifierName` that can be a direct child of `BindingProperty`
  /`AssignmentTargetPropertyProperty` is the key, and the old grandparent
  set always matched (`ObjectPattern`/`ObjectAssignmentTarget`), so that
  branch always fired. Matching only `PropertyKey::StaticIdentifier`
  preserves the computed-key exclusion.
- The `AssignmentTargetPropertyIdentifier` arm keeps the same
  parent-of-`ObjectAssignmentTarget` check. The old non-APTI path was dead:
  a rest target (`({...innerText} = node)`) goes through
  `AssignmentTargetRest`, never a direct `ObjectAssignmentTarget` child.

On the vscode codebase, violations are unchanged (245 warnings, 47 errors),
calls drop 76% (5,996,077 -> 1,437,765), and rule time drops ~80%
(116ms -> 23ms).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@connorshea
connorshea force-pushed the perf/prefer-dom-node-text-content-dispatch branch from e30436a to fb728a0 Compare June 30, 2026 03:57

@camc314 camc314 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

@camc314
camc314 merged commit dc8fd9a into main Jul 2, 2026
28 checks passed
@camc314
camc314 deleted the perf/prefer-dom-node-text-content-dispatch branch July 2, 2026 16:00
camc314 added a commit that referenced this pull request Jul 3, 2026
… run only on less common node types (#23897)

Somewhat similar to #23867 and #23868.

This was generated using Claude Code after I noticed this rule was one
of the slowest on the vscode repo. I have reviewed and tested the change
by running it on the vscode codebase, and the # of violations before and
after is identical. All tests also pass.

I've also had Claude run a battery of tests on code it generated to test
as many scenarios as possible, to ensure that there were no differences
in output on main vs. this branch, and confirmed that there were indeed
no differences.

## What changed

The rule's `NODE_TYPES` were already derived, but two of them —
`IdentifierName` and `IdentifierReference` — are among the most common
node types in any program (every property name, every variable read).
The destructuring cases dispatched on those identifiers and then walked
*up* to their parents to detect destructuring patterns.

This PR instead dispatches directly on the rare parent nodes and reads
the key/binding:

| Old dispatch (common) | New dispatch (rare) | Covers |
|---|---|---|
| `IdentifierName` | `BindingProperty` (key) +
`AssignmentTargetPropertyProperty` (name) | `const {innerText} = node`,
`({innerText: text} = node)` |
| `IdentifierReference` | `AssignmentTargetPropertyIdentifier` (binding)
| `({innerText} = node)` |

`StaticMemberExpression` (`node.innerText`) is unchanged.

## Why it's behavior-identical

- The only `IdentifierName` that can be a direct child of
`BindingProperty`/`AssignmentTargetPropertyProperty` is the key (values
bottom out in `BindingIdentifier`/`IdentifierReference`). The old
grandparent set always matched
(`ObjectPattern`/`ObjectAssignmentTarget`), so that branch always fired
— matching the new unconditional trigger. Matching only
`PropertyKey::StaticIdentifier` preserves the computed-key exclusion
(`{[innerText]: text}`).
- The `AssignmentTargetPropertyIdentifier` arm keeps the same
parent-of-`ObjectAssignmentTarget` check. The old non-APTI path was
dead: a rest target (`({...innerText} = node)`) goes through
`AssignmentTargetRest`, never a direct `ObjectAssignmentTarget` child.

The rule test snapshot is byte-identical (no `.snap` change).

## Results

Running `oxlint --debug=timings --quiet -A all -W
unicorn/prefer-dom-node-text-content` on the vscode repo.

Before:
```
Found 245 warnings and 47 errors.

Rule                                   Time (ms)  Relative    Calls  Source
------------------------------------  ----------  --------  -------  ------
unicorn/prefer-dom-node-text-content     116.624    100.0%  5996077  native
```

After:
```
Found 245 warnings and 47 errors.

Rule                                   Time (ms)  Relative    Calls  Source
------------------------------------  ----------  --------  -------  ------
unicorn/prefer-dom-node-text-content      23.631    100.0%  1437765  native
```

Violations are unchanged (245 warnings, 47 errors). Calls drop 76%
(5,996,077 → 1,437,765) and rule time drops ~80% (116ms → 23ms).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
Co-authored-by: Cameron <[email protected]>
camc314 added a commit that referenced this pull request Jul 3, 2026
… run only on less common node types (#23897)

Somewhat similar to #23867 and #23868.

This was generated using Claude Code after I noticed this rule was one
of the slowest on the vscode repo. I have reviewed and tested the change
by running it on the vscode codebase, and the # of violations before and
after is identical. All tests also pass.

I've also had Claude run a battery of tests on code it generated to test
as many scenarios as possible, to ensure that there were no differences
in output on main vs. this branch, and confirmed that there were indeed
no differences.

## What changed

The rule's `NODE_TYPES` were already derived, but two of them —
`IdentifierName` and `IdentifierReference` — are among the most common
node types in any program (every property name, every variable read).
The destructuring cases dispatched on those identifiers and then walked
*up* to their parents to detect destructuring patterns.

This PR instead dispatches directly on the rare parent nodes and reads
the key/binding:

| Old dispatch (common) | New dispatch (rare) | Covers |
|---|---|---|
| `IdentifierName` | `BindingProperty` (key) +
`AssignmentTargetPropertyProperty` (name) | `const {innerText} = node`,
`({innerText: text} = node)` |
| `IdentifierReference` | `AssignmentTargetPropertyIdentifier` (binding)
| `({innerText} = node)` |

`StaticMemberExpression` (`node.innerText`) is unchanged.

## Why it's behavior-identical

- The only `IdentifierName` that can be a direct child of
`BindingProperty`/`AssignmentTargetPropertyProperty` is the key (values
bottom out in `BindingIdentifier`/`IdentifierReference`). The old
grandparent set always matched
(`ObjectPattern`/`ObjectAssignmentTarget`), so that branch always fired
— matching the new unconditional trigger. Matching only
`PropertyKey::StaticIdentifier` preserves the computed-key exclusion
(`{[innerText]: text}`).
- The `AssignmentTargetPropertyIdentifier` arm keeps the same
parent-of-`ObjectAssignmentTarget` check. The old non-APTI path was
dead: a rest target (`({...innerText} = node)`) goes through
`AssignmentTargetRest`, never a direct `ObjectAssignmentTarget` child.

The rule test snapshot is byte-identical (no `.snap` change).

## Results

Running `oxlint --debug=timings --quiet -A all -W
unicorn/prefer-dom-node-text-content` on the vscode repo.

Before:
```
Found 245 warnings and 47 errors.

Rule                                   Time (ms)  Relative    Calls  Source
------------------------------------  ----------  --------  -------  ------
unicorn/prefer-dom-node-text-content     116.624    100.0%  5996077  native
```

After:
```
Found 245 warnings and 47 errors.

Rule                                   Time (ms)  Relative    Calls  Source
------------------------------------  ----------  --------  -------  ------
unicorn/prefer-dom-node-text-content      23.631    100.0%  1437765  native
```

Violations are unchanged (245 warnings, 47 errors). Calls drop 76%
(5,996,077 → 1,437,765) and rule time drops ~80% (116ms → 23ms).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
Co-authored-by: Cameron <[email protected]>
Boshen added a commit that referenced this pull request Jul 6, 2026
# Oxlint
### 🚀 Features

- 7db7a29 allocator: Add `ReplaceWith` trait (#24012) (overlookmotel)
- a2c97f3 linter/unicorn: Implement `explicit-timer-delay` rule (#23612)
(Mikhail Baev)
- 85735cb linter/unicorn: Implement `no-confusing-array-with` rule
(#23638) (Shekhu☺️)
- cb4fbb9 linter/eslint: Implement no-unreachable-loop rule (#23975)
(Todor Andonov)
- dc32112 linter/eslint/no-constant-binary-expression: Check relational
comparisons (#24088) (camc314)
- 439c344 linter/jsdoc: Added missing options to `jsdoc/require-param`
rule (#23364) (kapobajza)
- 62af717 linter/unicorn/filename-case: Add `lowercase` and
`screamingSnakeCase` (#24045) (Boshen)
- d963967 linter/unicorn/no-array-sort: Add `allowAfterSpread` option
(#24043) (Boshen)
- 0a75682 linter: Add per-rule timings for type-aware linting (#22488)
(camchenry)
- 743e222 linter/react: Add `disallowedValues` option for
`forbid-dom-props` rule (#23970) (Mikhail Baev)

### 🐛 Bug Fixes

- 7b80010 linter: Use direct binding symbol ids (#24216) (camc314)
- 8f94b49 linter/import/no-duplicates: Don't flag a type-only import
beside a side-effect import (#24030) (Boshen)
- d8c3fee linter/react/rules-of-hooks: Flag `useEffectEvent` escapes
(#23764) (Rayan Salhab)
- 0a7312b linter/no-deprecated-functions: Map `require.requireActual` to
`jest.requireActual` (#23627) (Jerry Zhao)
- d9e3ab3 linter/eslint/no-useless-return: Handle switch case
continuation (#23984) (camc314)
- 0b25582 ast: Type binding node `typeAnnotation` as `TSTypeAnnotation |
null` (#23113) (Boshen)
- 122d112 linter/eslint/no-restricted-imports: Flag dynamic import()
expressions (#24029) (Boshen)
- 59b6b83 linter: Avoid `OnceLock` re-entry on cyclic `export *`
re-exports (#23632) (Jerry Zhao)
- dd09af0 linter/import/namespace: Avoid panic on destructuring of an
unresolvable namespace re-export (#23626) (Jerry Zhao)
- bdb51c7 linter/jest/prefer-ending-with-an-expect: Validate config
patterns (#24122) (camc314)
- e383843 linter/unicorn/prefer-modern-dom-apis: Skip fixer for non
identifier arguments (#23630) (Jerry Zhao)
- 0ac4c83 linter: Detect circular config extends (#24115) (camc314)
- bae1edf linter/import/namespace: Check namespace imports after named
imports (#24094) (camc314)
- cd8fdfe linter/eslint/no-eval: Recognize Array.from family thisArg
(#24091) (camc314)
- 851ee43 linter/eslint/no-eval: Resolve this binding for functions
returned from an IIFE (#23643) (Jerry Zhao)
- 002ab35 linter/unicorn: Avoid prefer-array-find rest destructuring
false positive (#23654) (ColemanDunn)
- 01c8775 linter/unicorn/filename-case: Keep digits attached in
screamingSnakeCase (#24056) (Boshen)
- f256941 linter: Recognize `@effect/vitest` as a vitest import source
(#24025) (Boshen)
- 73eeb1d linter/import/extensions: Honor per-extension `never` for
explicit extensions (#24031) (Boshen)
- d4ebe1f linter: Reject non-object oxlint config files (#24026)
(Boshen)
- 45d607d linter/react/forbid-component-props: Make allow/disallow lists
optional in schema (#24024) (Boshen)
- 54076ad linter/unicorn/no-array-for-each: Suggest entries loop for
index callbacks (#24004) (camc314)
- d057736 linter/jsdoc: Avoid param root underflow (#23945) (camc314)
- 29c76bf linter/unicorn/prefer-at: Skip object numeric-key access
(#23909) (Gaurav Dubey)

### ⚡ Performance

- 657a8fc linter/oxc/bad-array-method-on-arguments: Only run on member
expressions instead of all identifiers (#24164) (camchenry)
- 073d9e7 linter/eslint/prefer-rest-params: Run on functions instead of
all identifiers (#24163) (camchenry)
- e5a4162 linter/jest/no-confusing-set-timeout: Early exit fast path
(#24092) (camc314)
- bca7ce5 linter: Only run react-perf rules on JSX attribute nodes
(#24083) (camchenry)
- 6881bf6 linter: Compute `apply_overrides` rule set lazily (#23648)
(Jerry Zhao)
- 911c106 linter/eslint/no-obj-calls: Use resolved reference instead of
scope walk (#23895) (Marius Schulz)
- dc8fd9a linter/unicorn/prefer-dom-node-text-content: Change dispatch
to run only on less common node types (#23897) (Connor Shea)
- fdbd34d linter/eslint/no-useless-call: Fast-path static callees
(#24077) (camc314)
- b1be114 linter/import/extensions: Skip empty config and borrow
extensions (#24075) (camc314)
- 4781b2d linter/eslint/no-obj-calls: Use direct global matches (#24076)
(camc314)
- e6cee89 linter: Avoid node-chain allocation for non-Jest calls
(#23907) (Yagiz Nizipli)
- 30dc517 linter/typescript/no-restricted-types: O(1) banned-type
lookups (#23827) (Yagiz Nizipli)

### 📚 Documentation

- 6ca9125 linter/typescript: Clarify consistent-type-imports behavior
(#23972) (camc314)
# Oxfmt
### 🚀 Features

- 4f4313e formatter_css: Update oxc-css-parser 0.0.5 (#24120) (leaysgur)
- 0ccd8a1 formatter_graphql: Update oxc-graphql-parser 0.0.5 (#24106)
(leaysgur)
- 89ec3d9 formatter_core: Add literal line and root indention primitives
(#24051) (leaysgur)
- 213a96b formatter_core: Add no-expand-parent for multiline text
(#24050) (leaysgur)
- 0e5bcc9 formatter_graphql: Update oxc-graphql-parser 0.0.4 (#24039)
(leaysgur)
- e0b35a1 formatter_css: Update `[email protected]` (#23974)
(leaysgur)

### 🐛 Bug Fixes

- 1fe6546 formatter: Omit unneeded `;` for type members with `no-semi`
(#24212) (leaysgur)
- 0ad7316 formatter: Print space for `ForStatement`.`update` only if
exists (#24211) (leaysgur)
- 3abbed5 formatter: Print `;` before jsdoc type-cast parens with
no-semi (#24208) (leaysgur)
- 9af3833 formatter_css: Make scss formatter consistent (#24207)
(leaysgur)
- 46d7194 formatter_css: Use fill IR for `@forward` members (#24206)
(leaysgur)
- e31038f formatter_css: Keep comment inside sass config list (#24205)
(leaysgur)
- d3b9591 formatter: Add parens around `await/yield` with `<T>` (#24202)
(leaysgur)
- 2121a55 oxfmt: Reuse tinypool process during the same LSP process
(#24197) (leaysgur)
- 9bf4b4a formatter_css: Align CSS output to Prettier 3.9.1 (#24100)
(leaysgur)
- cd2452e formatter_css: Align SCSS output to Prettier 3.9.1 (#24097)
(leaysgur)
- 4ee8745 formatter_css: Keep selector value contain line-break without
breaking line (#24055) (leaysgur)
- e1ece97 formatter_graphql: Break `implements` list by print-width
(#23997) (leaysgur)
- 0a6b16c formatter_json: Preserve key and literal value for
json-stringify (#23996) (leaysgur)
- 903ab6e formatter_css: Preserve newlines in css-in-js selector list
(#23992) (leaysgur)
- ea5d095 oxfmt: Update `--migrate prettier` (#23963) (leaysgur)

### ⚡ Performance

- 468e1e3 formatter_core: Make printer queues cursor-based (#24098)
(Boshen)
- c59f2fe rust: Return impl ExactSizeIterator from slice-backed
accessors (#24144) (Boshen)
- c292fb2 formatter: Inline fits element dispatcher (#23982) (camc314)

Co-authored-by: Boshen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants