perf(linter/no-this-in-sfc): reorder cheap name check, avoid String allocation#22164
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
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_nameto returnOption<&str>instead ofOption<String>to avoid per-thisallocation.
|
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. |
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.
443bb2b to
72bd846
Compare
…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)
# 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>
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:
is_potential_react_componentahead of other checks to avoid the more expensive checks on non-component functions.&strinstead of String to drop a few allocations.Claude's summary is below.
Two small changes to
react/no-this-in-sfc:is_potential_react_component— a first-char ASCII uppercase test — ahead of the ES5/ES6 ancestor walk and the nested-thiswalk. Non-PascalCase functions (the common case forthisin JSX files: class methods, jQuery-style helpers, validation callbacks) now skip both walks.&strfromget_function_nameinstead ofString. The only caller passes the result straight tois_react_component_name(&str), so theto_string()was pure overhead perthisexpression.Behavior is unchanged. Full JSON diagnostic output is byte-identical to
mainon the benchmark fixtures (onlystart_timediffers).Benchmark
Paired-sample interleaved bench (N=80, 8 warmup) against
main,--silent, on synthetic JSX fixtures: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.