Skip to content

resolver: store exports expansion_keys as indices instead of cloned subtrees - #32312

Closed
robobun wants to merge 5 commits into
mainfrom
farm/683560b2/exports-expansion-keys-index
Closed

robobun wants to merge 5 commits into
mainfrom
farm/683560b2/exports-expansion-keys-index

Conversation

@robobun

@robobun robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

The Rust port of the ExportsMap visitor's e_object arm deep-cloned the entire condition subtree for every wildcard export key. In src/resolver/package_json.rs the loop did:

if strings::ends_with(&key, b"/") || strings::contains_char(&key, b'*') {
    expansion_keys.push(MapEntry {
        value: value.clone(),   // <- recursive deep copy of Box<[u8]> / Box<[Entry]> / EntryDataMap
        key: key.clone(),
        key_range,
    });
}

whereas the Zig reference at package_json.zig:1192-1196 stores a shallow struct copy whose string / []const Entry payload aliases the same heap data already held by map_data.list.

Packages with many wildcard exports and nested import/require/node/default conditions (next, @mui/material, rxjs) therefore paid roughly 2x the heap for their exports map, and PackageJSON is interned in the process-lifetime DirInfo cache so the duplicate is never freed. Pure memory regression vs the Zig reference; no correctness change.

Fix

EntryDataMap::expansion_keys is now Box<[u32]> holding sorted indices into list, which expresses the same sharing the Zig code gets from aliased slices. The PATTERN_KEY_COMPARE sort looks keys up through map_data, and the single consumer in ESModule resolution dereferences the index before use.

Verification

New test/js/bun/resolve/exports-map-memory.test.ts builds two packages with identical 1000-key exports maps, one with wildcard keys and one without, resolves each in a fresh subprocess, and asserts the RSS delta of the wildcard package does not exceed the flat package's by more than 10%. A second test verifies wildcard resolution and PATTERN_KEY_COMPARE specificity ordering are unchanged.

build (wild - flat) / flat, before after
release ~0.26 ~0.00
debug+ASAN ~0.17 ~0.01

Existing resolve.test.ts wildcard tests, import-custom-condition.test.ts, and the bundler packagejson/ExportsPattern* suite all pass.

@coderabbitai

coderabbitai Bot commented Jun 15, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 1 second. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: bab240d2-8872-4d82-940b-9194f6f23e44

📥 Commits

Reviewing files that changed from the base of the PR and between 56907f5 and 3b0a124.

📒 Files selected for processing (1)
  • test/js/bun/resolve/exports-map-memory.test.ts

Walkthrough

EntryDataMap.expansion_keys is changed from Box<[MapEntry]> to Box<[u32]>, storing indices into the existing list instead of cloned entries. Parsing, sorting, and resolution lookup are updated accordingly. A new test file validates both memory efficiency (wildcard RSS overhead vs flat baseline) and pattern-matching specificity correctness.

Changes

exports map expansion_keys index-based refactor

Layer / File(s) Summary
EntryDataMap struct, parsing, and index sorting
src/resolver/package_json.rs
expansion_keys field type changed from Box<[MapEntry]> to Box<[u32]>. During parsing, the current list length is pushed as a u32 index instead of a cloned MapEntry. Sorting uses the referenced map_data[idx].key for glob specificity comparison.
resolve_imports_exports index lookup
src/resolver/package_json.rs
Resolution loop updated to iterate expansion_keys as u32 indices and retrieve each entry via map.list[idx as usize].
Memory regression and correctness tests
test/js/bun/resolve/exports-map-memory.test.ts
Adds probe.js RSS measurement, makeExports synthetic fixture generator, a memory test comparing wildcard vs flat-key RSS deltas, and a resolution correctness test verifying specificity ordering and trailing-slash handling.

Possibly related PRs

  • oven-sh/bun#31036: Modifies the same exports/imports expansion-key handling in package_json.rs, specifically addressing wildcard entry storage and cloning in the same Visitor::visit path that this PR refactors to index-based storage.

Suggested reviewers

  • Jarred-Sumner
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main optimization: changing how wildcard export keys are stored from cloned subtrees to indices.
Description check ✅ Passed The description thoroughly addresses both template sections with a detailed problem statement, fix explanation, and comprehensive verification results.
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.

@robobun

robobun commented Jun 15, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:03 AM PT - Jul 2nd, 2026

❌ @robobun, your commit 0762adc has some failures in Build #67946 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 32312

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

bun-32312 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 2 issues this PR may fix:

  1. [nextjs] [bun] High memory usage #18210 - Next.js high memory usage — Next.js has extensive wildcard exports patterns, exactly the scenario where the duplicated expansion_keys subtrees inflate RSS
  2. 3.6x memory usage over node while running NestJS application #4796 - 3.6x memory usage over Node with NestJS — NestJS pulls in many packages with wildcard exports; the interned exports map duplication contributes to the elevated RSS

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #18210
Fixes #4796

🤖 Generated with Claude Code

Comment thread test/js/bun/resolve/exports-map-memory.test.ts Outdated
Comment thread test/js/bun/resolve/exports-map-memory.test.ts
Comment thread test/js/bun/resolve/exports-map-memory.test.ts
Comment thread test/js/bun/resolve/exports-map-memory.test.ts Outdated

@coderabbitai coderabbitai 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.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/bun/resolve/exports-map-memory.test.ts`:
- Around line 33-42: The probe script in the exports-map-memory test has a broad
catch block that silently swallows any errors from Bun.resolveSync, allowing the
memory measurement to proceed even if the resolution fails. Remove the catch
block from the try statement and instead ensure that the tempDir setup includes
the actual target files that the resolver is expected to find, so that
Bun.resolveSync can succeed normally without needing error suppression. This
allows the subprocess to fail loudly if something is misconfigured, rather than
silently reporting a memory delta on a failed operation.
- Around line 53-57: The subprocess test is currently validating output and exit
code together with a throw statement, but should instead use expect assertions
in the proper order. Reorder the validation to first assert that stderr is empty
using expect(stderr).toBe(""), then validate the stdout output (checking that
delta is finite), and finally assert the exit code is 0. Apply this same
reordering pattern to both locations where spawned Bun processes using bunEnv
are validated, ensuring stderr assertions come before stdout and exitCode checks
to surface failures with useful diffs during test failures.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7444a33d-076b-46b5-abe6-4eabd8d6245b

📥 Commits

Reviewing files that changed from the base of the PR and between e0acad3 and 56907f5.

📒 Files selected for processing (2)
  • src/resolver/package_json.rs
  • test/js/bun/resolve/exports-map-memory.test.ts

Comment thread test/js/bun/resolve/exports-map-memory.test.ts
Comment thread test/js/bun/resolve/exports-map-memory.test.ts
@robobun

robobun commented Jun 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI status on build #67946 (final, after rebasing onto main for #33032): 285 test jobs passed, 0 test failures, 0 error annotations.

The single red job is darwin 26 aarch64 - test-bun, which failed before running any test:

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
Refusing to continue with a partial download (would silently fall back to the wrong binary).

This is agent/artifact infrastructure, not a test failure. The only other annotations are the flaky list (pre-existing Windows tests unrelated to the resolver: update_interactive_install, terminal-platform-gaps) and binary-size.

exports-map-memory.test.ts passed on every lane that ran it, including all the darwin lanes that completed. The diff is green and ready to merge; the darwin job just needs a retry from someone with Buildkite write access (my token is read-only and the retry API refused it).

robobun and others added 5 commits July 2, 2026 09:26
…ubtrees

The e_object arm of the ExportsMap visitor pushed a full MapEntry clone
into expansion_keys for every key ending in "/" or containing "*". The
derived Clone on Entry recursively deep-copies Box<[u8]>, Box<[Entry]>
and EntryDataMap, so each wildcard key paid a second full allocation of
its condition subtree. The Zig reference at package_json.zig:1192 stores
a shallow struct copy whose slice/pointer payload aliases map_data.list.

Store u32 indices into list instead, and dereference them at the single
consumer in ESModule resolution. PATTERN_KEY_COMPARE sorting now looks
up keys through map_data.
@robobun
robobun force-pushed the farm/683560b2/exports-expansion-keys-index branch from 3b0a124 to 0762adc Compare July 2, 2026 09:38
@robobun

robobun commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto main to resolve a conflict with #33032 (the SIMD JSON parser rewrite), which restructured the ExportsMap visitor: the e_object arm moved into a new visit_object method, visit now dispatches on E::JsonValue, and Entry lost its first_token field.

No conflict was semantic. The deep-clone bug was still present in the rewritten visit_object, so the same fix applies there: expansion_keys is Vec<u32> indices into map_data instead of cloned MapEntrys. The EntryDataMap struct and the ESModule consumer hunks applied cleanly.

Re-verified after the rebase: with main's src/resolver/package_json.rs the memory test fails (wild-flat 62.9MB over a 17.6MB threshold); with the fix both tests pass, and resolve.test.ts + import-custom-condition.test.ts are green.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-02, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

@robobun robobun closed this Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant