Skip to content

perf(parser): use SmallVec for duplicate default export detection#16801

Merged
graphite-app[bot] merged 1 commit intomainfrom
c/12-13-refactor_parser_use_smallvec_for_duplicate_default_export_detection
Dec 15, 2025
Merged

perf(parser): use SmallVec for duplicate default export detection#16801
graphite-app[bot] merged 1 commit intomainfrom
c/12-13-refactor_parser_use_smallvec_for_duplicate_default_export_detection

Conversation

@camc314
Copy link
Contributor

@camc314 camc314 commented Dec 13, 2025

Avoid heap allocation for the common case (0-2 default exports) by using
SmallVec with inline capacity of 2 instead of Vec.

🤖 generated with help from Claude Opus 4.5

@github-actions github-actions bot added A-parser Area - Parser C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Dec 13, 2025
Copy link
Contributor Author

camc314 commented Dec 13, 2025


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.

@camc314 camc314 changed the title refactor(parser): use SmallVec for duplicate default export detection perf(parser): use SmallVec for duplicate default export detection Dec 13, 2025
@github-actions github-actions bot added the C-performance Category - Solution not expected to change functional behavior, only performance label Dec 13, 2025
@camc314 camc314 force-pushed the c/12-13-refactor_parser_use_smallvec_for_duplicate_default_export_detection branch from 093c9b1 to 2b7836f Compare December 13, 2025 16:15
@camc314 camc314 marked this pull request as ready for review December 13, 2025 16:20
Copilot AI review requested due to automatic review settings December 13, 2025 16:20
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 13, 2025

CodSpeed Performance Report

Merging #16801 will not alter performance

Comparing c/12-13-refactor_parser_use_smallvec_for_duplicate_default_export_detection (7856ab2) with main (3f5d6b7)

Summary

✅ 42 untouched
⏩ 3 skipped1

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.

Copy link

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

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

Overall the change is small and reasonable, but the current SmallVec usage still eagerly collects all default-export spans. If you want the perf tweak to be maximally effective, consider short-circuiting once duplication is detected (or only collecting all labels when needed).

Additional notes (2)
  • Performance | crates/oxc_parser/src/module_record.rs:57-63
    SmallVec is introduced for a simple len() > 1 check, but this still collects all default-export spans. If a module has many default exports (pathological, but possible), this will still iterate and store everything (and will heap-allocate once it exceeds inline capacity). Since the only behavior difference is emitting an error with labels, consider an approach that short-circuits after the second hit for the common/valid case while still producing good diagnostics (e.g., collect first span, detect second, then optionally continue collecting only if you want all labels). This keeps the perf win but avoids extra work in invalid cases too.

  • Performance | crates/oxc_parser/src/module_record.rs:57-63
    The change to SmallVec is fine, but with_labels(default_exports) will typically consume an IntoIterator of labels. It’s safer and clearer to pass an iterator (or slice) explicitly, rather than relying on SmallVec’s trait impls staying compatible with the diagnostics API. This also avoids accidentally moving default_exports if future code wants to use it after with_labels.

Also, this function only needs to know whether there are 2+ defaults; collecting all spans is unnecessary work in degenerate cases with many exports. You can short-circuit once you’ve found a second default, while still preserving the labels you want to show.

Summary of changes

What changed

  • Added smallvec as a dependency for oxc_parser (crates/oxc_parser/Cargo.toml).
  • Switched duplicate default export span collection in ModuleRecordBuilder from std::vec::Vec to SmallVec<[Span; 2]> to avoid heap allocation in the common case (crates/oxc_parser/src/module_record.rs).
  • Updated imports to include use smallvec::SmallVec;.

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 optimizes memory allocation in the parser's duplicate default export detection by using SmallVec<[Span; 2]> instead of std::vec::Vec<_>. This avoids heap allocation for the common case where modules have 0-2 default exports (the typical scenario being 0 or 1 valid export).

Key Changes:

  • Replaced Vec with SmallVec with inline capacity of 2 for collecting default export spans
  • Added smallvec dependency to oxc_parser crate

Reviewed changes

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

File Description
crates/oxc_parser/src/module_record.rs Changed collection type from Vec to SmallVec<[Span; 2]> for default export spans
crates/oxc_parser/Cargo.toml Added smallvec workspace dependency
Cargo.lock Updated lock file to reflect new dependency

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

Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

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

default_exports is not used elsewhere, why not .count() 🤔 ?

@camc314
Copy link
Contributor Author

camc314 commented Dec 13, 2025

default_exports is not used elsewhere, why not .count() 🤔 ?

we use it for the labels in L63

@Boshen
Copy link
Member

Boshen commented Dec 13, 2025

@CharlieHelps change to use .count()?

@charliecreates
Copy link

charliecreates bot commented Dec 13, 2025

Expand this to see my work.
  • Previously refactored ModuleRecordBuilder::errors in crates/oxc_parser/src/module_record.rs to remove SmallVec, avoid allocations in the 0–1 default export case, and verified via cargo test -p oxc_parser --all-features and cargo ck that the parser and workspace still build and pass tests, then pushed commits 045d54a8a and 66081aa06 to the branch c/12-13-refactor_parser_use_smallvec_for_duplicate_default_export_detection referenced by PR #16801.
  • Implemented the reviewer-suggested .count() approach by introducing a default_export_spans iterator factory in module_record.rs and using default_export_spans().take(2).count() > 1 to detect duplicates before calling OxcDiagnostic::error("Duplicated default export").with_labels(default_export_spans()).
  • Ran rg over .github/workflows to confirm there’s no cargo fmt gate in CI, ensuring the new default_export_spans closure style won’t trip formatting checks.
  • Re-ran cargo ck after the .count() refactor and confirmed from the output that crates/oxc_parser and the rest of the workspace compile cleanly with the new duplicate default export logic.
  • Executed cargo test -p oxc_parser --all-features and confirmed all 54 tests, including module_record::module_record_tests::* covering export behavior, pass with the .count()-based implementation.
  • Committed and pushed commit 102071a2c (perf(parser): use count() to detect duplicate default exports) updating only crates/oxc_parser/src/module_record.rs on branch c/12-13-refactor_parser_use_smallvec_for_duplicate_default_export_detection, so PR #16801 now reflects the .count() approach Boshen asked about.

@charliecreates
Copy link

#16801 (comment)

Updated the PR to use .take(2).count() (vs collecting) to detect duplicate default exports, while still keeping the labels for the error case. Also dropped the smallvec dep since it’s no longer needed.

Changes
  • Switched duplicate-default detection to default_export_spans().take(2).count() > 1, and only builds labels when we actually have a duplicate (102071a2c, crates/oxc_parser/src/module_record.rs around L35-L70).
  • Removed smallvec from oxc_parser and the corresponding Cargo.lock entry (045d54a8).
Verification
# Cargo check: ok
$ cargo ck

# Tests (oxc_parser): 54 passed
$ cargo test -p oxc_parser --all-features

Skipped cargo test --workspace --all-features here because it fails on this devbox due to an oxc_codegen snapshot that includes the local Node.js version (unrelated to the parser change).

@Boshen Boshen force-pushed the c/12-13-refactor_parser_use_smallvec_for_duplicate_default_export_detection branch from 7856ab2 to ed80c3f Compare December 15, 2025 15:37
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Dec 15, 2025
Copy link
Member

Boshen commented Dec 15, 2025

Merge activity

@Boshen Boshen force-pushed the c/12-13-refactor_parser_use_smallvec_for_duplicate_default_export_detection branch from ed80c3f to 1ec15da Compare December 15, 2025 15:40
…6801)

Avoid heap allocation for the common case (0-2 default exports) by using
SmallVec with inline capacity of 2 instead of Vec.

🤖 generated with help from Claude Opus 4.5
@graphite-app graphite-app bot force-pushed the c/12-13-refactor_parser_use_smallvec_for_duplicate_default_export_detection branch from 1ec15da to 225f229 Compare December 15, 2025 15:52
@graphite-app graphite-app bot merged commit 225f229 into main Dec 15, 2025
21 checks passed
@graphite-app graphite-app bot deleted the c/12-13-refactor_parser_use_smallvec_for_duplicate_default_export_detection branch December 15, 2025 15:57
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 15, 2025
overlookmotel added a commit that referenced this pull request Dec 19, 2025
### 🚀 Features

- d209c21 allocator: Add cap to FixedSizeAllocatorPool and block when
exhausted (#17023) (Cameron)
- fb2af91 allocator: Add bitset utils (#17042) (zhaoting zhou)
- c16082c tasks/compat_data: Integrate `node-compat-table` (#16831)
(Boshen)
- 5586823 span: Extract TS declaration file check to its own function
(#17037) (camchenry)
- 3d2b492 minifier: Fold iife arrow functions in call expressions
(#16477) (Armano)
- 67e9f9e codegen: Keep comments on the export specifiers (#16943)
(夕舞八弦)
- cb515fa parser: Improve error message for `yield` as identifier usage
(#16950) (sapphi-red)
- dcc856b parser: Add help for `new_dynamic_import` error (#16949)
(sapphi-red)
- c3c79f8 parser: Improve import attribute value error message (#16948)
(sapphi-red)
- 291b57b ast_tools: Generate TS declaration files for deserializer and
walk files (#16912) (camc314)
- 74eae13 minifier: Remove unused import specifiers (#16797) (camc314)

### 🐛 Bug Fixes

- fb9e193 linter: OOM problems with custom plugins (#17082)
(overlookmotel)
- e59132b parser/napi: Fix lazy deser (#17069) (overlookmotel)
- a92faf0 ast_tools: Support `u128` in `assert_layouts` generator
(#17050) (overlookmotel)
- 47b4c2f minifier/docs: Correct hyperlink path in OPTIMIZATIONS.md
(#16986) (GRK)
- 3002649 transformer/typescript: Remove unused import equals
declaration (#16776) (Dunqing)
- 5a2af88 regular_expression: Correct named capture group reference
error (#16952) (sapphi-red)

### ⚡ Performance

- b657bb6 allocator: Reduce time `Mutex` lock is held in
`FixedSizeAllocatorPool::get` (#17079) (overlookmotel)
- 1f3b19b ast: `#[ast]` macro use `#[repr(transparent)]` for
single-field structs (#17052) (overlookmotel)
- 225f229 parser: Use SmallVec for duplicate default export detection
(#16801) (camc314)

### 📚 Documentation

- a9c419f traverse: Update safety comments (#16944) (overlookmotel)

Co-authored-by: overlookmotel <557937+overlookmotel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-parser Area - Parser C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants