refactor(binding): drop unsafe napi string helper, hoist transform ArcStr by hyf0 · Pull Request #9456 · rolldown/rolldown

hyf0 · 2026-05-19T06:32:52Z

Summary

Follow-up cleanups to #9436:

Lazy-init ArcStr::from(code.as_str()) in the transform hook (build_hooks.rs). Before this PR, transform allocated one ArcStr per plugin per module. The first pass of this PR hoisted that alloc out of the loop — but still paid one allocation per module even when no transform plugins were registered (regression vs. main). Now code_arc is an Option<ArcStr> initialized on first iteration and reset to None when a plugin replaces code, so the no-transform path stays a no-op and multi-plugin modules pay one alloc per code revision.
Use Arc::unwrap_or_clone in HookRenderChunkArgs::into_code.
Drop the create_napi_string unsafe helper in BindingSharedString — napi-rs already provides impl ToNapiValue for &str doing the same napi_create_string_utf8 call.

Codex review surfaced two issues with an earlier Arc::make_mut change in update_output_chunk (non-atomic on sourcemap conversion failure; full struct clone including the large code: String when the Arc is shared with the JS-side BindingOutputChunk). That change has been dropped from this PR.

Test plan

cargo check -p rolldown_binding -p rolldown_plugin passes
cargo clippy -p rolldown_binding -p rolldown_plugin passes (no new lints)
CI green

🤖 Generated with Claude Code

netlify · 2026-05-19T06:33:03Z

✅ Deploy Preview for rolldown-rs ready!

Name	Link
🔨 Latest commit	`4a2ac11`
🔍 Latest deploy log	https://app.netlify.com/projects/rolldown-rs/deploys/6a0c35d55f93880008496075
😎 Deploy Preview	https://deploy-preview-9456--rolldown-rs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull request overview

Follow-up cleanups to PR #9436 that simplify the binding/plugin code without changing observable behavior, while also fixing a regression where ArcStr::from(code.as_str()) was being allocated on every iteration of the per-plugin transform loop.

Changes:

Hoist ArcStr construction out of the transform plugin loop, re-allocating only when a plugin returns new code.
Replace Arc::get_mut + manual struct rebuild with Arc::make_mut; replace Arc::try_unwrap/clone fallback with Arc::unwrap_or_clone; introduce From<&ModuleId> for BindingSharedString to collapse repeated mappings.
Drop the custom create_napi_string unsafe helper in favor of napi-rs's built-in ToNapiValue for &str.

Reviewed changes

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

Show a summary per file

File	Description
crates/rolldown_plugin/src/types/hook_render_chunk_args.rs	Simplify `into_code` using `Arc::unwrap_or_clone`.
crates/rolldown_plugin/src/plugin_driver/build_hooks.rs	Hoist `ArcStr` allocation out of the per-plugin transform loop.
crates/rolldown_binding/src/types/binding_output_chunk.rs	Use `Arc::make_mut` to replace manual `get_mut`/rebuild branches.
crates/rolldown_binding/src/types/binding_module_info.rs	Add `From<&ModuleId>` impl and collapse repeated `.map(...)` constructions.
crates/rolldown_binding/src/options/plugin/types/binding_shared_string.rs	Remove custom `create_napi_string` unsafe helper; delegate to napi-rs `&str` impl.

codspeed-hq · 2026-05-19T06:38:51Z

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks¹

_{Comparing perf/simplify-binding-shared-string (44592a4) with main (2b0bb7f)}

10 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. ↩

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

hyf0 · 2026-05-19T07:26:50Z

Merge activity

May 19, 7:26 AM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
May 19, 10:04 AM UTC: hyf0 added this pull request to the Graphite merge queue.
May 19, 10:09 AM UTC: Merged by the Graphite merge queue.

…cStr (#9456) ## Summary Follow-up cleanups to #9436: - **Lazy-init `ArcStr::from(code.as_str())` in the transform hook** (`build_hooks.rs`). Before this PR, `transform` allocated one `ArcStr` per plugin per module. The first pass of this PR hoisted that alloc out of the loop — but still paid one allocation per module even when no transform plugins were registered (regression vs. main). Now `code_arc` is an `Option<ArcStr>` initialized on first iteration and reset to `None` when a plugin replaces `code`, so the no-transform path stays a no-op and multi-plugin modules pay one alloc per code revision. - **Use `Arc::unwrap_or_clone`** in `HookRenderChunkArgs::into_code`. - **Drop the `create_napi_string` unsafe helper** in `BindingSharedString` — napi-rs already provides `impl ToNapiValue for &str` doing the same `napi_create_string_utf8` call. Codex review surfaced two issues with an earlier `Arc::make_mut` change in `update_output_chunk` (non-atomic on sourcemap conversion failure; full struct clone including the large `code: String` when the Arc is shared with the JS-side `BindingOutputChunk`). That change has been dropped from this PR. ## Test plan - [x] `cargo check -p rolldown_binding -p rolldown_plugin` passes - [x] `cargo clippy -p rolldown_binding -p rolldown_plugin` passes (no new lints) - [ ] CI green 🤖 Generated with [Claude Code](https://claude.com/claude-code)

@IWANABETHATGUY

## [1.0.2] - 2026-05-20 ### 🚀 Features - devtools: emit package size in PackageGraphReady (#9434) by @IWANABETHATGUY - devtools: classify package dependency types (#9427) by @IWANABETHATGUY - devtools: map packages to modules and chunks (#9426) by @IWANABETHATGUY - devtools: mark used packages (#9423) by @IWANABETHATGUY - devtools: make duplicate packages discoverable (#9422) by @IWANABETHATGUY - devtools: emit package metadata (#9421) by @IWANABETHATGUY - update oxc to 0.132.0 (#9449) by @shulaoda - update oxc to 0.131.0 (#9424) by @shulaoda - allow checks.* to escalate emissions to hard errors (#9388) by @IWANABETHATGUY - dev: support watcher options `include` and `exclude` (#9395) by @h-a-n-a - emit warnings for invalid pure annotations (#9381) by @Kyujenius ### 🐛 Bug Fixes - hash: keep chunk file names stable when an unrelated entry is added (#9444) by @hyf0 - call `codeSplitting.groups[].name` in deterministic order (#9457) by @sapphi-red - dev/lazy: make `resolve_id` idempotent when the resolved id is already a lazy entry (#9439) by @h-a-n-a - chunk-optimization: publish absorbed dynamic-entry namespace cross-chunk (#9448) by @IWANABETHATGUY - treeshake: propagate pure annotation through compound exprs (#9431) by @Dunqing - finalizer: skip redundant init call when barrel executes in same chunk (#9354) by @IWANABETHATGUY - linking: initialize wrapped ESM re-export owners (#9353) by @IWANABETHATGUY - do not inherit __toESM across chunks for named-only external imports (#9333) (#9415) by @IWANABETHATGUY - watcher: don't write output or emit events after close() (#9328) by @situ2001 - chunk-optimization: avoid unsafe dynamic-only merges (#9398) by @IWANABETHATGUY - cjs: rename CJS-wrapped locals that would shadow chunk-scope names (#9392) by @hyf0 - dev/lazy: watch lazy modules added in rebuilds (#9391) by @h-a-n-a ### 🚜 Refactor - rolldown_dev: move dev example to break publish cycle (#9465) by @Boshen - binding: drop unsafe napi string helper, hoist transform ArcStr (#9456) by @hyf0 - ecmascript_utils: split rewrite_ident_reference off JsxExt trait (#9417) by @IWANABETHATGUY - use `ThreadsafeFunction::call_async_catch` (#9390) by @sapphi-red ### 📚 Documentation - devtools: document @rolldown/debug usage and package graph consumption (#9435) by @IWANABETHATGUY - replace `Inter` with system font stack in OG template SVG (#9240) by @yvbopeng - remove `output.comments` warning as all issues have been resolved (#9393) by @sapphi-red - in-depth: clarify @__PURE__ scope and document positions (#9389) by @Kyujenius - readme: remove release candidate notice (#9387) by @shulaoda ### ⚡ Performance - vite-resolve: cache importer existence checks (#9443) by @Brooooooklyn - binding: reduce plugin string clones (#9436) by @Brooooooklyn ### 🧪 Testing - enable `legal_comments_inline` test (#9394) by @sapphi-red ### ⚙️ Miscellaneous Tasks - bump pnpm to v11.1.2 (#9447) by @Boshen - deps: update rust crates (#9461) by @renovate[bot] - deps: update rollup submodule for tests to v4.60.4 (#9453) by @rolldown-guard[bot] - deps: update test262 submodule for tests (#9454) by @rolldown-guard[bot] - deps: update npm packages (#9430) by @renovate[bot] - deps: update github actions (#9429) by @renovate[bot] - deps: update dependency rolldown-plugin-dts to v0.25.1 (#9452) by @renovate[bot] - deps: update rust crates (#9428) by @renovate[bot] - revert allow checks.* to escalate emissions to hard errors (#9388) (#9438) by @IWANABETHATGUY - update mimalloc-safe to 0.1.61 (#9413) by @shulaoda - deny `todo`, `unimplemented`, and `print_stderr` clippy lints (#9412) by @Boshen - deps: update mimalloc-safe to 0.1.60 (#9410) by @shulaoda - remove `pip install setuptools` workaround for node-gyp on macOS (#9370) by @sapphi-red - renovate: disable automerge to require manual approval (#9386) by @shulaoda - deps: update napi (#9385) by @renovate[bot] ### ❤️ New Contributors * @yvbopeng made their first contribution in [#9240](#9240) Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>

…cStr (rolldown#9456) ## Summary Follow-up cleanups to rolldown#9436: - **Lazy-init `ArcStr::from(code.as_str())` in the transform hook** (`build_hooks.rs`). Before this PR, `transform` allocated one `ArcStr` per plugin per module. The first pass of this PR hoisted that alloc out of the loop — but still paid one allocation per module even when no transform plugins were registered (regression vs. main). Now `code_arc` is an `Option<ArcStr>` initialized on first iteration and reset to `None` when a plugin replaces `code`, so the no-transform path stays a no-op and multi-plugin modules pay one alloc per code revision. - **Use `Arc::unwrap_or_clone`** in `HookRenderChunkArgs::into_code`. - **Drop the `create_napi_string` unsafe helper** in `BindingSharedString` — napi-rs already provides `impl ToNapiValue for &str` doing the same `napi_create_string_utf8` call. Codex review surfaced two issues with an earlier `Arc::make_mut` change in `update_output_chunk` (non-atomic on sourcemap conversion failure; full struct clone including the large `code: String` when the Arc is shared with the JS-side `BindingOutputChunk`). That change has been dropped from this PR. ## Test plan - [x] `cargo check -p rolldown_binding -p rolldown_plugin` passes - [x] `cargo clippy -p rolldown_binding -p rolldown_plugin` passes (no new lints) - [ ] CI green 🤖 Generated with [Claude Code](https://claude.com/claude-code)

@IWANABETHATGUY

## [1.0.2] - 2026-05-20 ### 🚀 Features - devtools: emit package size in PackageGraphReady (rolldown#9434) by @IWANABETHATGUY - devtools: classify package dependency types (rolldown#9427) by @IWANABETHATGUY - devtools: map packages to modules and chunks (rolldown#9426) by @IWANABETHATGUY - devtools: mark used packages (rolldown#9423) by @IWANABETHATGUY - devtools: make duplicate packages discoverable (rolldown#9422) by @IWANABETHATGUY - devtools: emit package metadata (rolldown#9421) by @IWANABETHATGUY - update oxc to 0.132.0 (rolldown#9449) by @shulaoda - update oxc to 0.131.0 (rolldown#9424) by @shulaoda - allow checks.* to escalate emissions to hard errors (rolldown#9388) by @IWANABETHATGUY - dev: support watcher options `include` and `exclude` (rolldown#9395) by @h-a-n-a - emit warnings for invalid pure annotations (rolldown#9381) by @Kyujenius ### 🐛 Bug Fixes - hash: keep chunk file names stable when an unrelated entry is added (rolldown#9444) by @hyf0 - call `codeSplitting.groups[].name` in deterministic order (rolldown#9457) by @sapphi-red - dev/lazy: make `resolve_id` idempotent when the resolved id is already a lazy entry (rolldown#9439) by @h-a-n-a - chunk-optimization: publish absorbed dynamic-entry namespace cross-chunk (rolldown#9448) by @IWANABETHATGUY - treeshake: propagate pure annotation through compound exprs (rolldown#9431) by @Dunqing - finalizer: skip redundant init call when barrel executes in same chunk (rolldown#9354) by @IWANABETHATGUY - linking: initialize wrapped ESM re-export owners (rolldown#9353) by @IWANABETHATGUY - do not inherit __toESM across chunks for named-only external imports (rolldown#9333) (rolldown#9415) by @IWANABETHATGUY - watcher: don't write output or emit events after close() (rolldown#9328) by @situ2001 - chunk-optimization: avoid unsafe dynamic-only merges (rolldown#9398) by @IWANABETHATGUY - cjs: rename CJS-wrapped locals that would shadow chunk-scope names (rolldown#9392) by @hyf0 - dev/lazy: watch lazy modules added in rebuilds (rolldown#9391) by @h-a-n-a ### 🚜 Refactor - rolldown_dev: move dev example to break publish cycle (rolldown#9465) by @Boshen - binding: drop unsafe napi string helper, hoist transform ArcStr (rolldown#9456) by @hyf0 - ecmascript_utils: split rewrite_ident_reference off JsxExt trait (rolldown#9417) by @IWANABETHATGUY - use `ThreadsafeFunction::call_async_catch` (rolldown#9390) by @sapphi-red ### 📚 Documentation - devtools: document @rolldown/debug usage and package graph consumption (rolldown#9435) by @IWANABETHATGUY - replace `Inter` with system font stack in OG template SVG (rolldown#9240) by @yvbopeng - remove `output.comments` warning as all issues have been resolved (rolldown#9393) by @sapphi-red - in-depth: clarify @__PURE__ scope and document positions (rolldown#9389) by @Kyujenius - readme: remove release candidate notice (rolldown#9387) by @shulaoda ### ⚡ Performance - vite-resolve: cache importer existence checks (rolldown#9443) by @Brooooooklyn - binding: reduce plugin string clones (rolldown#9436) by @Brooooooklyn ### 🧪 Testing - enable `legal_comments_inline` test (rolldown#9394) by @sapphi-red ### ⚙️ Miscellaneous Tasks - bump pnpm to v11.1.2 (rolldown#9447) by @Boshen - deps: update rust crates (rolldown#9461) by @renovate[bot] - deps: update rollup submodule for tests to v4.60.4 (rolldown#9453) by @rolldown-guard[bot] - deps: update test262 submodule for tests (rolldown#9454) by @rolldown-guard[bot] - deps: update npm packages (rolldown#9430) by @renovate[bot] - deps: update github actions (rolldown#9429) by @renovate[bot] - deps: update dependency rolldown-plugin-dts to v0.25.1 (rolldown#9452) by @renovate[bot] - deps: update rust crates (rolldown#9428) by @renovate[bot] - revert allow checks.* to escalate emissions to hard errors (rolldown#9388) (rolldown#9438) by @IWANABETHATGUY - update mimalloc-safe to 0.1.61 (rolldown#9413) by @shulaoda - deny `todo`, `unimplemented`, and `print_stderr` clippy lints (rolldown#9412) by @Boshen - deps: update mimalloc-safe to 0.1.60 (rolldown#9410) by @shulaoda - remove `pip install setuptools` workaround for node-gyp on macOS (rolldown#9370) by @sapphi-red - renovate: disable automerge to require manual approval (rolldown#9386) by @shulaoda - deps: update napi (rolldown#9385) by @renovate[bot] ### ❤️ New Contributors * @yvbopeng made their first contribution in [rolldown#9240](rolldown#9240) Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 19, 2026 06:32

github-actions Bot assigned hyf0 May 19, 2026

Copilot started reviewing on behalf of hyf0 May 19, 2026 06:33 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

hyf0 requested a review from Copilot May 19, 2026 06:42

hyf0 changed the title ~~refactor(binding): simplify follow-ups to #9436~~ refactor(binding): drop unsafe napi string helper, hoist ArcStr, use Arc::make_mut May 19, 2026

Copilot started reviewing on behalf of hyf0 May 19, 2026 06:42 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

hyf0 force-pushed the perf/simplify-binding-shared-string branch 2 times, most recently from cd37bc2 to 44592a4 Compare May 19, 2026 07:17

hyf0 changed the title ~~refactor(binding): drop unsafe napi string helper, hoist ArcStr, use Arc::make_mut~~ refactor(binding): drop unsafe napi string helper, hoist transform ArcStr May 19, 2026

hyf0 requested a review from IWANABETHATGUY May 19, 2026 07:26

hyf0 added the graphite: merge-when-ready label May 19, 2026

IWANABETHATGUY approved these changes May 19, 2026

View reviewed changes

graphite-app Bot force-pushed the perf/simplify-binding-shared-string branch from 44592a4 to 4a2ac11 Compare May 19, 2026 10:05

graphite-app Bot merged commit 4a2ac11 into main May 19, 2026
32 checks passed

graphite-app Bot removed the graphite: merge-when-ready label May 19, 2026

graphite-app Bot deleted the perf/simplify-binding-shared-string branch May 19, 2026 10:09

rolldown-guard Bot mentioned this pull request May 20, 2026

release: v1.0.2 #9478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(binding): drop unsafe napi string helper, hoist transform ArcStr#9456

refactor(binding): drop unsafe napi string helper, hoist transform ArcStr#9456
graphite-app[bot] merged 1 commit into
mainfrom
perf/simplify-binding-shared-string

hyf0 commented May 19, 2026 •

edited

Loading

Uh oh!

netlify Bot commented May 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

codspeed-hq Bot commented May 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

hyf0 commented May 19, 2026 •

edited by graphite-app Bot

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hyf0 commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

netlify Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for rolldown-rs ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codspeed-hq Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

hyf0 commented May 19, 2026 • edited by graphite-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hyf0 commented May 19, 2026 •

edited

Loading

netlify Bot commented May 19, 2026 •

edited

Loading

codspeed-hq Bot commented May 19, 2026 •

edited

Loading

hyf0 commented May 19, 2026 •

edited by graphite-app Bot

Loading