optimize bundler cycle detection #25421
Conversation
WalkthroughAdds a module-loader benchmark script, simplifies base64 leftover checks to direct equality, implements a hybrid cycle-detection fast path and hash-set path in the bundler linker, and adds tests exercising several bundler cycle-detection scenarios. Changes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
bench/module-loader/run-benchmark.sh(1 hunks)src/base64/base64.zig(1 hunks)src/bundler/LinkerContext.zig(4 hunks)test/bundler/bundler_cycle_detection.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test,describe,expect) and import frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/bundler/bundler_cycle_detection.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/bundler/bundler_cycle_detection.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using-eflag overtempDir
For multi-file tests in Bun test suite, prefer usingtempDirandBun.spawn
Always useport: 0when spawning servers in tests - do not hardcode ports or use custom random port functions
UsenormalizeBunSnapshotto normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
UsetempDirfromharnessto create temporary directories in tests - do not usetmpdirSyncorfs.mkdtempSync
In tests, callexpect(stdout).toBe(...)beforeexpect(exitCode).toBe(0)when spawning processes for more useful error messages on failure
Do not write flaky tests - do not usesetTimeoutin tests; insteadawaitthe condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails withUSE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>- tests are not valid if they pass withUSE_SYSTEM_BUN=1
Avoid shell commands in tests - do not usefindorgrep; use Bun's Glob and built-in tools instead
Test files must end in.test.tsor.test.tsxand be created in the appropriate test folder structure
Files:
test/bundler/bundler_cycle_detection.test.ts
src/**/*.{cpp,zig}
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
src/**/*.{cpp,zig}: Usebun bdorbun run build:debugto build debug versions for C++ and Zig source files; creates debug build at./build/debug/bun-debug
Run tests usingbun bd test <test-file>with the debug build; never usebun testdirectly as it will not include your changes
Execute files usingbun bd <file> <...args>; never usebun <file>directly as it will not include your changes
Enable debug logs for specific scopes usingBUN_DEBUG_$(SCOPE)=1environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required
Files:
src/bundler/LinkerContext.zigsrc/base64/base64.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Use
bun.Output.scoped(.${SCOPE}, .hidden)for creating debug logs in Zig codeImplement core functionality in Zig, typically in its own directory in
src/
src/**/*.zig: Private fields in Zig are fully supported using the#prefix:struct { #foo: u32 };
Use decl literals in Zig for declaration initialization:const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer@importat the bottom of the file (auto formatter will move them automatically)Be careful with memory management in Zig code - use defer for cleanup with allocators
Files:
src/bundler/LinkerContext.zigsrc/base64/base64.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.zig: Expose generated bindings in Zig structs usingpub const js = JSC.Codegen.JS<ClassName>with trait conversion methods:toJS,fromJS, andfromJSDirect
Use consistent parameter nameglobalObjectinstead ofctxin Zig constructor and method implementations
Usebun.JSError!JSValuereturn type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup usingdeinit()method that releases resources, followed byfinalize()called by the GC that invokesdeinit()and frees the pointer
UseJSC.markBinding(@src())in finalize methods for debugging purposes before callingdeinit()
For methods returning cached properties in Zig, declare external C++ functions usingextern fnandcallconv(JSC.conv)calling convention
Implement getter functions with naming patternget<PropertyName>in Zig that acceptthisandglobalObjectparameters and returnJSC.JSValue
Access JavaScript CallFrame arguments usingcallFrame.argument(i), check argument count withcallFrame.argumentCount(), and getthiswithcallFrame.thisValue()
For reference-counted objects, use.deref()in finalize instead ofdestroy()to release references to other JS objects
Files:
src/bundler/LinkerContext.zigsrc/base64/base64.zig
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/bundle.test.ts : Organize bundle tests in bundle.test.ts for tests concerning bundling bugs that only occur in DevServer
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Add tests for new Bun runtime functionality
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/esm.test.ts : Organize ESM tests in esm.test.ts for tests about various ESM features in development mode
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/bundle.test.ts : Organize bundle tests in bundle.test.ts for tests concerning bundling bugs that only occur in DevServer
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/css.test.ts : Organize CSS tests in css.test.ts for tests concerning bundling bugs with CSS files
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/esm.test.ts : Organize ESM tests in esm.test.ts for tests about various ESM features in development mode
Applied to files:
test/bundler/bundler_cycle_detection.test.tsbench/module-loader/run-benchmark.sh
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/sourcemap.test.ts : Organize source-map tests in sourcemap.test.ts for tests verifying source-maps are correct
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/ecosystem.test.ts : Organize ecosystem tests in ecosystem.test.ts for tests involving ensuring certain libraries are correct; prefer testing concrete bugs over testing entire packages
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/**/*.{js,ts,jsx,tsx} : Write tests as JavaScript and TypeScript files using Jest-style APIs (`test`, `describe`, `expect`) and import from `bun:test`
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/**/*.test.ts : Specify expected bundling errors via the `errors` option when a client is expected to encounter an error; by default, a client opening a page to an error fails the test
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : For multi-file tests in Bun test suite, prefer using `tempDir` and `Bun.spawn`
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/bundler/LinkerContext.zigsrc/base64/base64.zig
📚 Learning: 2025-09-07T14:00:36.526Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22258
File: src/bun.js/test/jest.zig:63-86
Timestamp: 2025-09-07T14:00:36.526Z
Learning: In Bun's Zig codebase, the established pattern for HashMap hash functions is to truncate hash values to u32, even when the underlying hash computation produces u64. This is seen throughout the codebase, such as in src/css/rules/rules.zig where u64 hashes are stored but truncated to u32 in HashMap contexts. When implementing HashContext for HashMapUnmanaged, the hash function should return u32 to match this pattern.
Applied to files:
src/bundler/LinkerContext.zig
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for C++ JavaScript classes
Applied to files:
src/bundler/LinkerContext.zig
📚 Learning: 2025-11-10T00:57:09.173Z
Learnt from: franciscop
Repo: oven-sh/bun PR: 24514
File: src/bun.js/api/crypto/PasswordObject.zig:86-101
Timestamp: 2025-11-10T00:57:09.173Z
Learning: In Bun's Zig codebase (PasswordObject.zig), when validating the parallelism parameter for Argon2, the upper limit is set to 65535 (2^16 - 1) rather than using `std.math.maxInt(u24)` because the latter triggers Zig's truncation limit checks. The value 65535 is a practical upper bound that avoids compiler issues while being sufficient for thread parallelism use cases.
Applied to files:
src/base64/base64.zig
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/base64/base64.zig
🧬 Code graph analysis (1)
test/bundler/bundler_cycle_detection.test.ts (1)
test/bundler/expectBundled.ts (1)
itBundled(1758-1792)
🔇 Additional comments (6)
src/base64/base64.zig (1)
270-280: LGTM! Redundant modulo operations removed.Since
leftover = source_len % 4is already in range[0, 3], the additional% 4operations were redundant. The simplified equality checks are functionally equivalent and slightly more efficient.src/bundler/LinkerContext.zig (2)
1839-1874: LGTM! Hybrid approach balances small-cycle efficiency with large-cycle scalability.The optimization correctly:
- Uses O(n) array scan for small cycles (< 8 elements) avoiding hash map overhead
- Lazily initializes the hash set only when needed
- Properly defers cleanup of the hash set
1882-1887: Verify: Tracker is appended to array but also added to hash set.The tracker is appended to
cycle_detector(line 1882) regardless of whether the hash path is active. Then ifcycle_set.count() > 0, it's also added to the hash set (lines 1885-1887). This is correct behavior - the array is needed forre_exportsdependencies, and the hash set is for O(1) cycle detection.test/bundler/bundler_cycle_detection.test.ts (2)
3-130: LGTM! Comprehensive test coverage for cycle detection optimization.The test suite effectively covers:
- ShallowCycleDetection (4 depth): Exercises the array scan path (< 8 elements)
- DeepCycleDetection (11 depth): Exercises the hash set path (>= 8 elements)
- DeepChainNoCycle: Validates deep import chains without cycles still work
- DeepCrossFileCycle: Tests cycle detection across multiple files
This aligns well with the threshold of 8 in the implementation.
1-3: Thedescribefunction does not require an explicit import in Bun test files. Bun's test runner automatically injects Jest-like globals (describe,test,expect) into test files during transpilation, making explicit imports optional. The code is correct as-is.bench/module-loader/run-benchmark.sh (1)
1-34: Ensure./bench/module-loader/import.mjsexists in the repository.The benchmark script references
./bench/module-loader/import.mjsfor running benchmarks and needs this file to be present for the script to function correctly.
| START=$(date +%s%3N) | ||
| ./build/debug/bun-debug ./bench/module-loader/import.mjs > /tmp/bench-output.txt 2>&1 | ||
| EXIT_CODE=$? | ||
| END=$(date +%s%3N) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider portability of date +%s%3N for milliseconds.
The %3N format specifier for nanoseconds is a GNU extension and may not work on BSD/macOS date. For broader compatibility, consider using:
- START=$(date +%s%3N)
+ START=$(python3 -c 'import time; print(int(time.time() * 1000))')Alternatively, document that GNU coreutils is required.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| START=$(date +%s%3N) | |
| ./build/debug/bun-debug ./bench/module-loader/import.mjs > /tmp/bench-output.txt 2>&1 | |
| EXIT_CODE=$? | |
| END=$(date +%s%3N) | |
| START=$(python3 -c 'import time; print(int(time.time() * 1000))') | |
| ./build/debug/bun-debug ./bench/module-loader/import.mjs > /tmp/bench-output.txt 2>&1 | |
| EXIT_CODE=$? | |
| END=$(date +%s%3N) |
🤖 Prompt for AI Agents
In bench/module-loader/run-benchmark.sh around lines 13 to 16, the script uses
the GNU-only date format `date +%s%3N` to capture milliseconds which fails on
BSD/macOS; replace it with a portable approach such as calling Python/Perl to
compute epoch milliseconds (for example invoke a small one-liner that prints
int(time.time()*1000)) or detect and prefer GNU `gdate` if present, and add a
clear fallback/error that documents the dependency if neither option is
available; ensure both START and END use the same portable method and preserve
existing exit-code capture and timing calculation.
src/bundler/LinkerContext.zig
Outdated
| /// Hash context for ImportTracker used in cycle detection | ||
| const ImportTrackerHashCtx = struct { | ||
| pub fn hash(_: @This(), tracker: ImportTracker) u64 { | ||
| // Combine source_index (u32) and import_ref hash (u64) | ||
| // name_loc is not included in the hash as it doesn't affect semantic equality | ||
| const source_hash = @as(u64, tracker.source_index.get()); | ||
| const ref_hash = tracker.import_ref.hash64(); | ||
| return source_hash ^ ref_hash; | ||
| } | ||
|
|
||
| pub fn eql(_: @This(), a: ImportTracker, b: ImportTracker) bool { | ||
| return a.source_index.get() == b.source_index.get() and | ||
| a.import_ref.eql(b.import_ref); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider improving hash distribution.
The XOR of a small source_index (u32 cast to u64) with a full u64 ref_hash may not distribute well when source indices are small (common case). Consider mixing more thoroughly:
pub fn hash(_: @This(), tracker: ImportTracker) u64 {
- const source_hash = @as(u64, tracker.source_index.get());
- const ref_hash = tracker.import_ref.hash64();
- return source_hash ^ ref_hash;
+ var h: u64 = tracker.source_index.get();
+ h = h *% 0x9E3779B97F4A7C15; // golden ratio mixing
+ h ^= tracker.import_ref.hash64();
+ return h;
}That said, for cycle detection the collision rate is unlikely to cause correctness issues since eql is called on collisions, and performance impact should be minimal given the typical cycle sizes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Hash context for ImportTracker used in cycle detection | |
| const ImportTrackerHashCtx = struct { | |
| pub fn hash(_: @This(), tracker: ImportTracker) u64 { | |
| // Combine source_index (u32) and import_ref hash (u64) | |
| // name_loc is not included in the hash as it doesn't affect semantic equality | |
| const source_hash = @as(u64, tracker.source_index.get()); | |
| const ref_hash = tracker.import_ref.hash64(); | |
| return source_hash ^ ref_hash; | |
| } | |
| pub fn eql(_: @This(), a: ImportTracker, b: ImportTracker) bool { | |
| return a.source_index.get() == b.source_index.get() and | |
| a.import_ref.eql(b.import_ref); | |
| } | |
| }; | |
| /// Hash context for ImportTracker used in cycle detection | |
| const ImportTrackerHashCtx = struct { | |
| pub fn hash(_: @This(), tracker: ImportTracker) u64 { | |
| // Combine source_index (u32) and import_ref hash (u64) | |
| // name_loc is not included in the hash as it doesn't affect semantic equality | |
| var h: u64 = tracker.source_index.get(); | |
| h = h *% 0x9E3779B97F4A7C15; // golden ratio mixing | |
| h ^= tracker.import_ref.hash64(); | |
| return h; | |
| } | |
| pub fn eql(_: @This(), a: ImportTracker, b: ImportTracker) bool { | |
| return a.source_index.get() == b.source_index.get() and | |
| a.import_ref.eql(b.import_ref); | |
| } | |
| }; |
|
Thanks for this Was the benchmark in a release build? |
Yes @Jarred-Sumner |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
LGTM if bundler tests pass
There will be a few unrelated test failures our CI needs some fixes
cirospaciari
left a comment
cirospaciari
left a comment
There was a problem hiding this comment.
LGTM lets wait CI before merge
cirospaciari
left a comment
cirospaciari
left a comment
There was a problem hiding this comment.
looks like the test test/bundler/bundler_cycle_detection.test.ts is failing in all platforms, the other failures are not related
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/bundler/bundler_cycle_detection.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
test/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test,describe,expect) and import frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/bundler/bundler_cycle_detection.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/bundler/bundler_cycle_detection.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using-eflag overtempDir
For multi-file tests in Bun test suite, prefer usingtempDirandBun.spawn
Always useport: 0when spawning servers in tests - do not hardcode ports or use custom random port functions
UsenormalizeBunSnapshotto normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
UsetempDirfromharnessto create temporary directories in tests - do not usetmpdirSyncorfs.mkdtempSync
In tests, callexpect(stdout).toBe(...)beforeexpect(exitCode).toBe(0)when spawning processes for more useful error messages on failure
Do not write flaky tests - do not usesetTimeoutin tests; insteadawaitthe condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails withUSE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>- tests are not valid if they pass withUSE_SYSTEM_BUN=1
Avoid shell commands in tests - do not usefindorgrep; use Bun's Glob and built-in tools instead
Test files must end in.test.tsor.test.tsxand be created in the appropriate test folder structure
Files:
test/bundler/bundler_cycle_detection.test.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/bundle.test.ts : Organize bundle tests in bundle.test.ts for tests concerning bundling bugs that only occur in DevServer
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Add tests for new Bun runtime functionality
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/esm.test.ts : Organize ESM tests in esm.test.ts for tests about various ESM features in development mode
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `process.platform` and `process.arch` for platform detection; these values are inlined and dead-code eliminated at build time
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/bundle.test.ts : Organize bundle tests in bundle.test.ts for tests concerning bundling bugs that only occur in DevServer
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/css.test.ts : Organize CSS tests in css.test.ts for tests concerning bundling bugs with CSS files
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/esm.test.ts : Organize ESM tests in esm.test.ts for tests about various ESM features in development mode
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/ecosystem.test.ts : Organize ecosystem tests in ecosystem.test.ts for tests involving ensuring certain libraries are correct; prefer testing concrete bugs over testing entire packages
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/sourcemap.test.ts : Organize source-map tests in sourcemap.test.ts for tests verifying source-maps are correct
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/**/*.{js,ts,jsx,tsx} : Write tests as JavaScript and TypeScript files using Jest-style APIs (`test`, `describe`, `expect`) and import from `bun:test`
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/**/*.test.ts : Specify expected bundling errors via the `errors` option when a client is expected to encounter an error; by default, a client opening a page to an error fails the test
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : For multi-file tests in Bun test suite, prefer using `tempDir` and `Bun.spawn`
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced
Applied to files:
test/bundler/bundler_cycle_detection.test.ts
🧬 Code graph analysis (1)
test/bundler/bundler_cycle_detection.test.ts (1)
test/bundler/expectBundled.ts (1)
itBundled(1758-1792)
| import { itBundled } from "./expectBundled"; | ||
|
|
||
| describe("bundler", () => { | ||
| // Test basic cycle detection (uses array scan path, < 8 depth) | ||
| itBundled("bundler/ShallowCycleDetection", { | ||
| files: { | ||
| "/entry.js": /* js */ ` | ||
| export {a as b} from './entry' | ||
| export {b as c} from './entry' | ||
| export {c as d} from './entry' | ||
| export {d as a} from './entry' | ||
| `, | ||
| }, | ||
| bundleErrors: { | ||
| "/entry.js": [ | ||
| `Detected cycle while resolving import "a"`, | ||
| `Detected cycle while resolving import "b"`, | ||
| `Detected cycle while resolving import "c"`, | ||
| `Detected cycle while resolving import "d"`, | ||
| ], | ||
| }, | ||
| }); | ||
|
|
||
| // Test deep cycle detection (should use hash set path, >= 8 depth) | ||
| itBundled("bundler/DeepCycleDetection", { | ||
| files: { | ||
| "/entry.js": /* js */ ` | ||
| export {a as b} from './entry' | ||
| export {b as c} from './entry' | ||
| export {c as d} from './entry' | ||
| export {d as e} from './entry' | ||
| export {e as f} from './entry' | ||
| export {f as g} from './entry' | ||
| export {g as h} from './entry' | ||
| export {h as i} from './entry' | ||
| export {i as j} from './entry' | ||
| export {j as k} from './entry' | ||
| export {k as a} from './entry' | ||
| `, | ||
| }, | ||
| bundleErrors: { | ||
| "/entry.js": [ | ||
| `Detected cycle while resolving import "a"`, | ||
| `Detected cycle while resolving import "b"`, | ||
| `Detected cycle while resolving import "c"`, | ||
| `Detected cycle while resolving import "d"`, | ||
| `Detected cycle while resolving import "e"`, | ||
| `Detected cycle while resolving import "f"`, | ||
| `Detected cycle while resolving import "g"`, | ||
| `Detected cycle while resolving import "h"`, | ||
| `Detected cycle while resolving import "i"`, | ||
| `Detected cycle while resolving import "j"`, | ||
| `Detected cycle while resolving import "k"`, | ||
| ], | ||
| }, | ||
| }); | ||
|
|
||
| // Test deep chain without cycle (should work correctly) | ||
| itBundled("bundler/DeepChainNoCycle", { | ||
| files: { | ||
| "/entry.js": /* js */ ` | ||
| import { x } from './a.js'; | ||
| console.log(x); | ||
| `, | ||
| "/a.js": /* js */ ` | ||
| import { x } from './b.js'; | ||
| export { x }; | ||
| `, | ||
| "/b.js": /* js */ ` | ||
| import { x } from './c.js'; | ||
| export { x }; | ||
| `, | ||
| "/c.js": /* js */ ` | ||
| import { x } from './d.js'; | ||
| export { x }; | ||
| `, | ||
| "/d.js": /* js */ ` | ||
| import { x } from './e.js'; | ||
| export { x }; | ||
| `, | ||
| "/e.js": /* js */ ` | ||
| import { x } from './f.js'; | ||
| export { x }; | ||
| `, | ||
| "/f.js": /* js */ ` | ||
| import { x } from './g.js'; | ||
| export { x }; | ||
| `, | ||
| "/g.js": /* js */ ` | ||
| import { x } from './h.js'; | ||
| export { x }; | ||
| `, | ||
| "/h.js": /* js */ ` | ||
| export const x = 42; | ||
| `, | ||
| }, | ||
| run: { | ||
| stdout: "42", | ||
| }, | ||
| }); | ||
|
|
||
| // Test cross-file cycle detection with deep chain | ||
| itBundled("bundler/DeepCrossFileCycle", { | ||
| files: { | ||
| "/entry.js": /* js */ ` | ||
| export {a as b} from './foo1' | ||
| export {b as c} from './foo1' | ||
| `, | ||
| "/foo1.js": /* js */ ` | ||
| export {c as d} from './foo2' | ||
| export {d as e} from './foo2' | ||
| `, | ||
| "/foo2.js": /* js */ ` | ||
| export {e as f} from './foo3' | ||
| export {f as g} from './foo3' | ||
| `, | ||
| "/foo3.js": /* js */ ` | ||
| export {g as h} from './foo4' | ||
| export {h as i} from './foo4' | ||
| `, | ||
| "/foo4.js": /* js */ ` | ||
| export {i as j} from './entry' | ||
| export {j as a} from './entry' | ||
| `, | ||
| }, | ||
| bundleErrors: { | ||
| "/entry.js": [`Detected cycle while resolving import "a"`, `Detected cycle while resolving import "b"`], | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Excellent test coverage for cycle detection optimization!
The test suite comprehensively validates the hybrid cycle-detection optimization:
- ShallowCycleDetection: Validates the array scan path (< 8 depth)
- DeepCycleDetection: Validates the hash-set path (>= 8 depth)
- DeepChainNoCycle: Confirms no false positives on deep non-cyclic chains
- DeepCrossFileCycle: Tests real-world cross-file cycle scenarios
The tests follow all coding guidelines: proper use of itBundled, clear naming, no timing dependencies, and well-organized under describe("bundler").
Optional enhancement: Consider adding an edge-case test with exactly 8 items in the cycle to validate the threshold boundary behavior:
// Test threshold boundary (exactly 8 depth)
itBundled("bundler/ThresholdCycleDetection", {
files: {
"/entry.js": /* js */ `
export {a as b} from './entry'
export {b as c} from './entry'
export {c as d} from './entry'
export {d as e} from './entry'
export {e as f} from './entry'
export {f as g} from './entry'
export {g as h} from './entry'
export {h as a} from './entry'
`,
},
bundleErrors: {
"/entry.js": [
`Detected cycle while resolving import "a"`,
`Detected cycle while resolving import "b"`,
`Detected cycle while resolving import "c"`,
`Detected cycle while resolving import "d"`,
`Detected cycle while resolving import "e"`,
`Detected cycle while resolving import "f"`,
`Detected cycle while resolving import "g"`,
`Detected cycle while resolving import "h"`,
],
},
});🤖 Prompt for AI Agents
test/bundler/bundler_cycle_detection.test.ts lines 1-130: add a new test case
"bundler/ThresholdCycleDetection" that creates a cycle of exactly 8 exports
(a→b→c→d→e→f→g→h→a) and asserts bundleErrors list contains "Detected cycle while
resolving import" for each of a..h; insert it alongside the other itBundled
cases so the suite validates the threshold boundary between the array-scan and
hash-set cycle-detection paths.
It seems I've written the include path wrong in the first place. Can you rerun CI @cirospaciari? |
3 participants
Improves performance of bundler by 2% on large projects