Skip to content

perf(linter/no-this-in-sfc): reorder cheap name check, avoid String allocation#22164

Merged
graphite-app[bot] merged 1 commit into
mainfrom
perf/no-this-in-sfc-reorder
May 6, 2026
Merged

perf(linter/no-this-in-sfc): reorder cheap name check, avoid String allocation#22164
graphite-app[bot] merged 1 commit into
mainfrom
perf/no-this-in-sfc-reorder

Conversation

@connorshea

Copy link
Copy Markdown
Member

This was generated and benchmarked using Claude Code, reviewed and tested by me.

Ultimately, this is basically micro-benching, but I've made sure the benchmarks show this is a pretty universal improvement and the change is simple enough that I think it's also a code improvement/simplification regardless.

Two simple changes:

  • Move is_potential_react_component ahead of other checks to avoid the more expensive checks on non-component functions.
  • Return &str instead of String to drop a few allocations.

Claude's summary is below.


Two small changes to react/no-this-in-sfc:

  1. Move is_potential_react_component — a first-char ASCII uppercase test — ahead of the ES5/ES6 ancestor walk and the nested-this walk. Non-PascalCase functions (the common case for this in JSX files: class methods, jQuery-style helpers, validation callbacks) now skip both walks.
  2. Return &str from get_function_name instead of String. The only caller passes the result straight to is_react_component_name(&str), so the to_string() was pure overhead per this expression.

Behavior is unchanged. Full JSON diagnostic output is byte-identical to main on the benchmark fixtures (only start_time differs).

Benchmark

Paired-sample interleaved bench (N=80, 8 warmup) against main, --silent, on synthetic JSX fixtures:

Fixture main patch Δ t-stat
Combined (~136K LoC) 47.9 ms 46.4 ms −3.2% +7.76
Non-PascalCase helpers (worst case for main) 21.4 ms 19.8 ms −8.2% +14.4
Class methods 25.6 ms 24.9 ms −2.8% +5.29
Pascal SFCs (diagnostic-firing) 44.7 ms 44.0 ms −1.5% +4.12
Parse-only differential 25.2 ms 25.0 ms −0.9% +1.68 (n.s.)

Rule-attributable share on the combined fixture: −1.3 ms (−5.7% of the rule's own cost). Parse-only differential is not significant, so the wall-clock signal isn't binary-layout drift.

TL;DR −3.2% on combined fixture, −8.2% on worst case, parity confirmed.

@github-actions github-actions Bot added the A-linter Area - Linter label May 6, 2026
@codspeed-hq

codspeed-hq Bot commented May 6, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 47 skipped benchmarks1


Comparing perf/no-this-in-sfc-reorder (443bb2b) with main (3f81147)2

Open in CodSpeed

Footnotes

  1. 47 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.

  2. No successful run was found on main (e6ddc9c) during the generation of this report, so 3f81147 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@connorshea connorshea marked this pull request as ready for review May 6, 2026 02:33
@connorshea connorshea requested a review from camc314 as a code owner May 6, 2026 02:33
Copilot AI review requested due to automatic review settings May 6, 2026 02:33

Copilot AI 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.

Pull request overview

This PR micro-optimizes the react/no-this-in-sfc linter rule by avoiding unnecessary work and allocations when analyzing this usage inside JSX files, while keeping diagnostics unchanged.

Changes:

  • Reorders the cheap “potential React component name” check ahead of the more expensive ancestor and nested-context walks to short-circuit earlier in common non-component cases.
  • Changes get_function_name to return Option<&str> instead of Option<String> to avoid per-this allocation.

@camchenry

Copy link
Copy Markdown
Member

Thanks! I think these changes are pretty reasonable even without a benchmark, since I know these are generally worthwhile improvements. But it's nice to have the numbers to back it up too.

@camchenry camchenry added the 0-merge Merge with Graphite Merge Queue label May 6, 2026

camchenry commented May 6, 2026

Copy link
Copy Markdown
Member

Merge activity

…llocation (#22164)

This was generated and benchmarked using Claude Code, reviewed and tested by me.

Ultimately, this is basically micro-benching, but I've made sure the benchmarks show this is a pretty universal improvement and the change is simple enough that I think it's also a code improvement/simplification regardless.

Two simple changes:

- Move `is_potential_react_component` ahead of other checks to avoid the more expensive checks on non-component functions.
- Return `&str` instead of String to drop a few allocations.

Claude's summary is below.

---

Two small changes to `react/no-this-in-sfc`:

  1. Move `is_potential_react_component` — a first-char ASCII uppercase test — ahead of the ES5/ES6 ancestor walk and the nested-`this` walk. Non-PascalCase functions (the common case for `this` in JSX files: class methods, jQuery-style helpers, validation callbacks) now skip both walks.
  2. Return `&str` from `get_function_name` instead of `String`. The only caller passes the result straight to `is_react_component_name(&str)`, so the `to_string()` was pure overhead per `this` expression.

  Behavior is unchanged. Full JSON diagnostic output is byte-identical to `main` on the benchmark fixtures (only `start_time` differs).

  ### Benchmark

  Paired-sample interleaved bench (N=80, 8 warmup) against `main`, `--silent`, on synthetic JSX fixtures:

  | Fixture | main | patch | Δ | t-stat |
  |---|---|---|---|---|
  | Combined (~136K LoC) | 47.9 ms | 46.4 ms | **−3.2%** | +7.76 |
  | Non-PascalCase helpers (worst case for main) | 21.4 ms | 19.8 ms | **−8.2%** | +14.4 |
  | Class methods | 25.6 ms | 24.9 ms | −2.8% | +5.29 |
  | Pascal SFCs (diagnostic-firing) | 44.7 ms | 44.0 ms | −1.5% | +4.12 |
  | Parse-only differential | 25.2 ms | 25.0 ms | −0.9% | +1.68 (n.s.) |

  Rule-attributable share on the combined fixture: **−1.3 ms (−5.7% of the rule's own cost)**. Parse-only differential is not significant, so the wall-clock signal isn't binary-layout drift.

TL;DR −3.2% on combined fixture, −8.2% on worst case, parity confirmed.
@graphite-app graphite-app Bot force-pushed the perf/no-this-in-sfc-reorder branch from 443bb2b to 72bd846 Compare May 6, 2026 03:37
@graphite-app graphite-app Bot merged commit 72bd846 into main May 6, 2026
28 checks passed
@graphite-app graphite-app Bot deleted the perf/no-this-in-sfc-reorder branch May 6, 2026 03:41
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label May 6, 2026
graphite-app Bot pushed a commit that referenced this pull request May 6, 2026
…h rfind (#22172)

Another minor optimization + code simplification. Claude's summary is below. As with all of these, the benchmark is based on running the rule on a file with a lot of specific code that isn't likely to occur in real codebases. So the % drops shouldn't be seen as representative of much beyond "this specific code path is faster".

---

## Summary

Two allocations per `@property` JSDoc tag with a dot — the `Vec<&str>` from `split + collect` and the `String` from `join(".")` — collapse to a single slice index using `rfind('.')`.

```rust
// before
if type_name.contains('.') {
    let mut parts = type_name.split('.').collect::<Vec<_>>();
    parts.pop();                                // `foo[].bar` -> `foo[]`
    let parent_name = parts.join(".");
    let parent_name = parent_name.trim_end_matches("[]");
    ...
}

// after
if let Some(dot_idx) = type_name.rfind('.') {
    let parent_name = type_name[..dot_idx].trim_end_matches("[]");  // `foo[].bar` -> `foo[]` -> `foo`
    ...
}
```

`rfind('.')` matches the last dot; the original `split + pop + join` reassembled all-but-the-last segment with `.` as the separator — equivalent.

Same family as #22164/#22166/#22167/#22168.

## Benchmark

Synthetic fixture: 30 JS files × 60 typedef blocks × 16 `@property` tags ≈ **28,800 tags**, ~80% with dots. Counting global allocator (system alloc, no mimalloc) on the rule-on and parse-only configs; rule-attributable = total − parse-only.

### Wall-clock (paired interleaved, N=60)

| | Baseline | Optimized | Δ |
|---|---|---|---|
| Rule ON | 21.564 ms | 20.550 ms | **−1.014 ms** |
| Parse-only | 7.319 ms | 7.326 ms | +0.007 ms (noise) |

- Paired t-stat: **5.02** (rule-on); **−0.05** (parse-only control)
- Win/Loss/Tie: **42 / 18 / 0**
- Parse-only is flat, so the delta isn't a binary-layout artifact.

### Allocations (rule-attributable)

| Metric | Baseline | Optimized | Saved | Reduction |
|---|---|---|---|---|
| Count | 318,002 | 282,603 | **35,399** | **11.1%** |
| Bytes | 30.7 MB | 29.4 MB | **1.32 MB** | **4.3%** |

Saves ~1.5 allocations per dotted tag on this fixture (Vec is sometimes elided when the path is `a.b`, but `String` from `join` always allocates).

## Test plan

- [x] `cargo test -p oxc_linter --lib jsdoc::check_property_names` — passes
- [x] `--format=json` parity confirmed across baseline and optimized binaries on the fixture (only `start_time` differs)
- [x] Parse-only control confirms no binary-layout effect (t = −0.05)
- [x] `just fmt` clean

🤖 Generated with [Claude Code](https://claude.com/claude-code)
camc314 added a commit that referenced this pull request May 11, 2026
# Oxlint
### 💥 BREAKING CHANGES

- 00ce512 oxlint/lsp: [**BREAKING**] Don't fix suggestions on fixAll
code actions & command (#22195) (Sysix)

### 🚀 Features

- 0eeceaf linter/no-unused-vars: Rename parameter with initializer
(#22308) (camc314)
- fa0232b linter/no-unused-vars: Add param rename suggestion (#22285)
(Ryota Misumi)
- ae59305 linter/promise/no-promise-in-callback: Add
`exemptDeclarations` option (#22275) (Mikhail Baev)
- 60bed4a linter: Extends `no-redundant-roles` and
`prefer-tag-over-role` support roles (#22069) (mehm8128)
- 545c80f linter/eslint: Implement `prefer-regex-literals` rule (#22192)
(Mikhail Baev)
- cf86d7a linter: Bulk suppression (#19328) (Said Atrahouch)
- 23abd22 linter/jsx-a11y: Implement
no-noninteractive-element-to-interactive-role (#21264) (Pedro Tainha)
- fbb8f22 linter: Support `ignores` in overrides (#22148) (camc314)
- 5a4414d oxlint/lsp: Support `rulesCustomization` lsp option (#21858)
(Sysix)

### 🐛 Bug Fixes

- 610f4c7 linter/no-unused-vars: Avoid renaming captured vars (#22310)
(camc314)
- 6b50f23 oxlint/cli: Load root config by searching up parent
directories (#22272) (Sysix)
- 31a5de7 linter: Rename override `ignores` to `excludeFiles` (#22283)
(camc314)
- 26d5d7b linter: Add missing vitest/valid-describe-callback
functionality (#22279) (camchenry)
- 784530f linter: `valid-title`: detect `String.raw` strings (#22271)
(Sysix)
- 080d90e linter: Move `no-debugger` fix to suggestion (#22256) (Sysix)
- 25b7017 linter: Undocument override `ignores` option (#22213)
(camc314)
- 7bb00dd linter: Fix role-has-required-aria-props (#22097) (mehm8128)
- d25279e linter/disable-directives: Improve parsing of names,
descriptions (#22184) (camc314)
- a59e447 linter/disable-directives: Ignore invalid enable suffixes
(#22179) (camc314)
- aafef0f ci: Disable bulk supression test on big endian (#22175)
(camc314)
- 281daec linter/vue/define-props-destructuring: Add
`only-when-assigned` config opt (#22142) (camc314)
- 46ab679 linter/plugins: Trim leading newline for partial sources
(#20928) (bab)
- 29ff6d9 linter: Update docs for no_alias_methods rule to be
Vitest-specific and add toThrowError alias (#22129) (camchenry)

### ⚡ Performance

- 9414bee linter/role-has-required-aria-props: Avoid intermediate vec
(#22212) (camc314)
- 3883ea3 linter/no-useless-escape: Drop unnecessary Vec collect
(#22171) (connorshea)
- 42c3029 linter/check-property-names: Replace split-collect-pop-join
with rfind (#22172) (connorshea)
- 9551d53 linter: Remove unnecessary Vec collect in CFG edge traversal
(#22167) (connorshea)
- 26fa2fc linter/aria-role: Remove unnecessary string allocations in run
method (#22168) (connorshea)
- c9ce045 linter/getter-return: Remove unnecessary Vec collect in CFG
edge traversal (#22166) (connorshea)
- 72bd846 linter/no-this-in-sfc: Reorder cheap name check, avoid String
allocation (#22164) (connorshea)

### 📚 Documentation

- 4da212a linter/no-unused-vars: Add docs to
`rename_unused_function_parameter` (#22311) (camc314)
- 27c4628 linter/forbid-dom-props: Escape jsx examples in lint rule docs
(#22254) (4MBL)
- 3f81147 linter: Improve the `react/jsx-key` rule docs. (#22162)
(connorshea)
- 07f03cc linter/consistent-return: Add note about `noImplicitReturns`
coverage (#22156) (camc314)
- 7c1e049 oxlint/lsp: Improve autogenerated lsp docs (#22154) (Sysix)
- 87b3e38 linter: Update docs to be vitest-specific for
consistent-test-it (#22128) (camchenry)
# Oxfmt
### 💥 BREAKING CHANGES

- 5c6c390 oxfmt: [**BREAKING**] Respect more git ignore options, align
with Oxlint (#22210) (leaysgur)

### 🚀 Features

- 6e8e818 oxfmt: Experimental .svelte support (#21700) (leaysgur)

### 🐛 Bug Fixes

- e2a20b6 formatter: Add space after commas in import attributes
(#22274) (Leonabcd123)

### ⚡ Performance

- b756682 oxfmt: Optimize nested config prescan (#22232) (Jovi De
Croock)
- f14e81e formatter/sort_imports: Skip sort for single import runs
(#22204) (leaysgur)
- 32255b1 formatter: Process `ImportDeclaration`s in a run (#22079)
(overlookmotel)

### 📚 Documentation

- 4da6f4c formatter: Correct comment (#22217) (overlookmotel)
- ef3507d formatter/sort_imports: Refresh docs (#22203) (leaysgur)

Co-authored-by: Cameron <cameron.clark@hey.com>
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.

3 participants