Skip to content

resolver: avoid unconditional deep clone of exports Entry in visit() - #31036

Merged
Jarred-Sumner merged 1 commit into
mainfrom
claude/clone-pkgjson-entry
May 19, 2026
Merged

Jarred-Sumner merged 1 commit into
mainfrom
claude/clone-pkgjson-entry

Conversation

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

package.json exports-map visit() (EObject arm) deep-cloned each Entry (recursive Box<[Entry]> tree) before deciding whether to keep it. Borrow first, clone only when retained. ~193 recursive clones / ~8 KB on the rolldown 20K bundle.

Found via runtime clone tracer (claude/clone-tracer).

The EObject arm of the exports-tree builder pushed a deep clone of every
(key, value) into map_data and then conditionally moved the original into
expansion_keys. Because Entry/EntryData is recursively owned in Rust
(Box<[u8]>/Vec<MapEntry>), the unconditional clone walks the entire
subtree for every key.

Swap the order: clone into expansion_keys only when the key is a pattern
(ends in '/' or contains '*'), and always move into map_data. map_data
push order is unchanged; expansion_keys is sorted immediately after so
its push order is irrelevant. The Zig sibling does a shallow struct copy
here (borrowed slices), so no deep duplication was ever intended.
@robobun

robobun commented May 19, 2026 •

Copy link
Copy Markdown
Collaborator
Updated 5:42 AM PT - May 19th, 2026

❌ @Jarred-Sumner, your commit 830515c has 3 failures in Build #56054 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31036

That installs a local version of the PR into your bun-31036 executable, so you can run:

bun-31036 --bun

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e6ac2830-9d11-4ad3-ba13-4d556884ee7a

📥 Commits

Reviewing files that changed from the base of the PR and between f816284 and 830515c.

📒 Files selected for processing (1)
  • src/resolver/package_json.rs

Walkthrough

This PR optimizes ownership handling in JSON object visitation within the package resolver. The change moves key and value into the primary map_data list and reserves cloning for expansion_keys entries that contain wildcards or directory-like paths.

Changes

JSON object property ownership

Layer / File(s) Summary
JSON object property ownership optimization
src/resolver/package_json.rs
Visitor::visit for ExprData::EObject now moves key and value into map_data and conditionally clones them only when adding to expansion_keys for wildcard or directory-like keys, reducing unnecessary clones.
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description lacks required template sections. It is missing explicit 'What does this PR do?' and 'How did you verify your code works?' sections specified in the repository template. Add explicit sections following the template: structure the explanation under 'What does this PR do?' and add a 'How did you verify your code works?' section documenting verification steps.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main optimization: avoiding unnecessary deep cloning of exports Entry in the visit() function, which is the core change in the PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@claude claude Bot 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.

LGTM — pure reordering so the clone only happens in the conditional expansion_keys branch.

Extended reasoning...

Overview

This PR touches a single function in src/resolver/package_json.rs — the EObject arm of Visitor::visit() that parses package.json exports maps. Previously, every iteration cloned key and value (a recursive Entry tree) into map_data first, then conditionally moved the originals into expansion_keys. The change swaps the order: the conditional expansion_keys.push now does the clone, and the unconditional map_data.push consumes the originals by move. Net effect: clones only occur for keys ending in / or containing *, instead of for every key.

Security risks

None. No new inputs, no parsing changes, no control-flow changes — just which side of the two pushes pays the clone cost.

Level of scrutiny

Low. This is a mechanical refactor with identical observable behavior. Both map_data and expansion_keys end up with the same MapEntry contents in the same order as before. The only thing that could go wrong is if something between the old and new push positions depended on map_data already containing the current entry — I checked, and the only map_data indexed access (map_data[i - 1] at line 1835) happens earlier in the loop body and references the previous iteration's entry, which is unaffected.

Other factors

The key borrow used in the strings::ends_with / contains_char check still works because key hasn't been moved yet at that point. key_range is Copy. No CODEOWNERS entry covers this path. The change matches the PR description's stated intent (avoid ~193 recursive clones on the rolldown bundle) exactly.

@Jarred-Sumner
Jarred-Sumner merged commit 5deb4ca into main May 19, 2026
76 of 79 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the claude/clone-pkgjson-entry branch May 19, 2026 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants