Skip to content

perf(semantic): remove hash when checking identifier#17564

Merged
graphite-app[bot] merged 1 commit intomainfrom
01-01-perf_semantic_remove_hash_when_checking_identifier
Jan 1, 2026
Merged

perf(semantic): remove hash when checking identifier#17564
graphite-app[bot] merged 1 commit intomainfrom
01-01-perf_semantic_remove_hash_when_checking_identifier

Conversation

@camchenry
Copy link
Member

@camchenry camchenry commented Jan 1, 2026 •
edited
Loading

phf_set! is good for large lists of strings, but in cases of small lists, it is often faster to just do a .contains or a matches!. It also slightly improves compile times since no work needs to be done to hash at compile time.

It's possible the math on this might flip once we implement Ident (i.e., #16416), since the hashes will be pre-computed at parse-time.

@github-actions github-actions bot added A-semantic Area - Semantic C-performance Category - Solution not expected to change functional behavior, only performance labels Jan 1, 2026
@camchenry Graphite App
Copy link
Member Author

camchenry commented Jan 1, 2026

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 1, 2026 •
edited
Loading

CodSpeed Performance Report

Merging #17564 will improve performance by 9.08%

Comparing 01-01-perf_semantic_remove_hash_when_checking_identifier (fac3d94) with main (9e8ec78)

Summary

⚡ 3 improvements
✅ 39 untouched
⏩ 3 skipped1

Benchmarks breakdown

Mode Benchmark BASE HEAD Efficiency
⚡ Simulation semantic[cal.com.tsx] 23.3 ms 22.1 ms +5.57%
⚡ Simulation semantic[react.development.js] 1.4 ms 1.3 ms +6.75%
⚡ Simulation semantic[binder.ts] 3.7 ms 3.4 ms +9.08%

Footnotes

  1. 3 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 reviewed Jan 1, 2026
View reviewed changes
Copy link
Contributor

@camc314 camc314 left a comment

Choose a reason for hiding this comment

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

using matches! might be faster? - the compiler should optimize it pretty nicely into a jump table

@camchenry camchenry force-pushed the 01-01-perf_semantic_remove_hash_when_checking_identifier branch from 2d628c1 to 597f97e Compare January 1, 2026 18:51
@camchenry
Copy link
Member Author

camchenry commented Jan 1, 2026

using matches! might be faster? - the compiler should optimize it pretty nicely into a jump table

okay well it turns out that binary_search was not good in this case, just a simple contains was much fewer instructions. I'm going to try the matches! and see if that's comparable, otherwise I'm going with contains.

Screenshot 2026-01-01 at 1 59 05 PM

@camchenry camchenry force-pushed the 01-01-perf_semantic_remove_hash_when_checking_identifier branch from 597f97e to fac3d94 Compare January 1, 2026 19:02
@camc314
Copy link
Contributor

camc314 commented Jan 1, 2026

CodSpeed Performance Report

Merging #17564 will improve performance by 8.88%

Comparing 01-01-perf_semantic_remove_hash_when_checking_identifier (597f97e) with main (9e8ec78)

Summary

⚡ 4 improvements ✅ 38 untouched ⏩ 3 skipped1

Benchmarks breakdown

Mode Benchmark BASE HEAD Efficiency
⚡ Simulation semantic[RadixUIAdoptionSection.jsx] 66.2 µs 64.3 µs +3.01%
⚡ Simulation semantic[cal.com.tsx] 23.3 ms 22.1 ms +5.5%
⚡ Simulation semantic[binder.ts] 3.7 ms 3.4 ms +8.88%
⚡ Simulation semantic[react.development.js] 1.4 ms 1.4 ms +6.72%

Footnotes

  1. 3 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. ↩

This is amazing!

@camchenry
Copy link
Member Author

camchenry commented Jan 1, 2026 •
edited
Loading

Looks like the matches! is a touch faster for medium to large files, and just very slightly slower for the radix benchmark, but only like 0.2 microseconds, so not worth fretting too much about since it's still nearly +3% overall.

Screenshot 2026-01-01 at 2 13 31 PM

@camchenry camchenry marked this pull request as ready for review January 1, 2026 19:14
@camchenry camchenry requested a review from Dunqing as a code owner January 1, 2026 19:14
Copilot AI review requested due to automatic review settings January 1, 2026 19:14
camc314
camc314 approved these changes Jan 1, 2026
View reviewed changes
Copy link
Contributor

@camc314 camc314 left a comment

Choose a reason for hiding this comment

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

awesome work!

Copilot AI reviewed Jan 1, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

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 improves performance and compilation time by replacing the phf_set! macro with a simple match expression for checking strict mode reserved identifiers. For small lists like the 9 strict mode reserved words, direct pattern matching is more efficient than hashing.

Key Changes:

  • Replaced STRICT_MODE_NAMES phf set with inline match pattern for strict mode identifier validation
  • Refactored check_identifier to use a single match expression for both "await" and strict mode checks
  • Removed phf dependency from oxc_semantic crate

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
crates/oxc_semantic/src/checker/javascript.rs Refactored identifier checking logic to use match expressions instead of phf_set, removed STRICT_MODE_NAMES constant
crates/oxc_semantic/Cargo.toml Removed phf dependency from both dependencies and dev-dependencies
Cargo.lock Updated lockfile to reflect removal of phf from oxc_semantic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@camchenry camchenry added the 0-merge Merge with Graphite Merge Queue label Jan 1, 2026
@camchenry Graphite App
Copy link
Member Author

camchenry commented Jan 1, 2026 •
edited by graphite-app bot
Loading

Merge activity

@camchenry
perf(semantic): remove hash when checking identifier (#17564)
6067143 
`phf_set!` is good for large lists of strings, but in cases of small lists, it is often faster to just do a `.contains` or a `matches!`. It also slightly improves compile times since no work needs to be done to hash at compile time.

It's possible the math on this might flip once we implement `Ident` (i.e., #16416), since the hashes will be pre-computed at parse-time.
@graphite-app graphite-app bot force-pushed the 01-01-perf_semantic_remove_hash_when_checking_identifier branch from fac3d94 to 6067143 Compare January 1, 2026 19:18
@graphite-app graphite-app bot merged commit 6067143 into main Jan 1, 2026
21 checks passed
@graphite-app graphite-app bot deleted the 01-01-perf_semantic_remove_hash_when_checking_identifier branch January 1, 2026 19:24
@Boshen Boshen mentioned this pull request Jan 5, 2026
Merged
graphite-app bot pushed a commit that referenced this pull request Jan 5, 2026
@Boshen
release(crates): oxc v0.107.0 (#17661)
811b2a7 
### 🚀 Features

- 659c23e linter: Init note field boilerplate  (#17589) (Shrey Sudhir)
- 6870b64 parser: Add TS1363 error code (#17609) (Sysix)
- 23680a3 mangler: Skip mangling only in scopes affected by direct eval (#17612) (camc314)
- a7e1643 parser: Add TS2528 error code to duplicate_default_export diagnostic (#17558) (camc314)

### 🐛 Bug Fixes

- 1044116 ecmascript: Mark `new Symbol` as non side-effect free (#17568) (camc314)
- ab5e4ca isolated-declarations: Strip default values from rest parameter binding patterns (#17602) (camc314)
- 68b2e54 minifier: Prevent incorrect ??= transformation when member base is mutated (#17472) (copilot-swe-agent)

### ⚡ Performance

- 6067143 semantic: Remove hash when checking identifier (#17564) (camchenry)
- a28ab3d semantic: Avoid bounds check when checking string literal (#17545) (camc314)
- 04809d1 semantic: Use SIMD for finding backslashes in `check_string_literal` (#17534) (camchenry)
- 49ad2f0 semantic: Mark all diagnostic functions as `#[cold]` (#17487) (camc314)
- ea82b50 transformer: Mark all diagnostic functions as `#[cold]` (#17486) (camc314)
- d968e51 semantic: Mark `checker::check` as `inline(always)` (#17459) (camc314)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@camc314 camc314 camc314 approved these changes

@Dunqing Dunqing Awaiting requested review from Dunqing Dunqing is a code owner

Assignees

No one assigned

Labels

0-merge Merge with Graphite Merge Queue A-semantic Area - Semantic C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@camchenry @camc314