Skip to content

Commit 8c50565

Browse files
anonriganonrig
committed
optimize bundler cycle detection
1 parent 3af0d23 commit 8c50565

File tree

4 files changed

+211
-6
lines changed

4 files changed

+211
-6
lines changed

‎bench/module-loader/run-benchmark.sh‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
#!/bin/bash
2+
# Benchmark script for cycle detection optimization
3+
4+
echo "=== Module Loader Benchmark ==="
5+
echo "Testing with 500 files, deep re-export chains"
6+
echo ""
7+
8+
# Run the benchmark 3 times and take the best result
9+
BEST_TIME=999999
10+
11+
for i in 1 2 3; do
12+
echo "Run $i/3..."
13+
START=$(date +%s%3N)
14+
./build/debug/bun-debug ./bench/module-loader/import.mjs > /tmp/bench-output.txt 2>&1
15+
EXIT_CODE=$?
16+
END=$(date +%s%3N)
17+
18+
if [ $EXIT_CODE -eq 0 ]; then
19+
ELAPSED=$((END - START))
20+
echo " Time: ${ELAPSED}ms"
21+
22+
if [ $ELAPSED -lt $BEST_TIME ]; then
23+
BEST_TIME=$ELAPSED
24+
fi
25+
else
26+
echo " Failed with exit code $EXIT_CODE"
27+
cat /tmp/bench-output.txt
28+
fi
29+
done
30+
31+
echo ""
32+
echo "Best time: ${BEST_TIME}ms"
33+
echo ""
34+
grep "Loaded" /tmp/bench-output.txt

‎src/base64/base64.zig‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -271,9 +271,9 @@ const zig_base64 = struct {
271271
var result = source_len / 4 * 3;
272272
const leftover = source_len % 4;
273273
if (decoder.pad_char != null) {
274-
if (leftover % 4 != 0) return error.InvalidPadding;
274+
if (leftover != 0) return error.InvalidPadding;
275275
} else {
276-
if (leftover % 4 == 1) return error.InvalidPadding;
276+
if (leftover == 1) return error.InvalidPadding;
277277
result += leftover * 3 / 4;
278278
}
279279
return result;

‎src/bundler/LinkerContext.zig‎

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1836,6 +1836,10 @@ pub const LinkerContext = struct {
18361836
var result: MatchImport = MatchImport{};
18371837
const named_imports = c.graph.ast.items(.named_imports);
18381838

1839+
const hash_threshold = 8;
1840+
var cycle_set: std.HashMapUnmanaged(ImportTracker, void, ImportTrackerHashCtx, std.hash_map.default_max_load_percentage) = .{};
1841+
defer cycle_set.deinit(c.allocator());
1842+
18391843
loop: while (true) {
18401844
// Make sure we avoid infinite loops trying to resolve cycles:
18411845
//
@@ -1844,10 +1848,26 @@ pub const LinkerContext = struct {
18441848
// export {b as c} from './foo.js'
18451849
// export {c as a} from './foo.js'
18461850
//
1847-
// This uses a O(n^2) array scan instead of a O(n) map because the vast
1848-
// majority of cases have one or two elements
1849-
for (c.cycle_detector.items[cycle_detector_top..]) |prev_tracker| {
1850-
if (std.meta.eql(tracker, prev_tracker)) {
1851+
// Optimization:
1852+
// - O(n) array scan for small sizes (< 8 elements),
1853+
// - O(1) hash set lookup for larger sizes
1854+
const cycle_slice = c.cycle_detector.items[cycle_detector_top..];
1855+
if (cycle_slice.len < hash_threshold) {
1856+
for (cycle_slice) |prev_tracker| {
1857+
if (ImportTrackerHashCtx.eql(.{}, tracker, prev_tracker)) {
1858+
result = .{ .kind = .cycle };
1859+
break :loop;
1860+
}
1861+
}
1862+
} else {
1863+
if (cycle_set.count() == 0) {
1864+
bun.handleOom(cycle_set.ensureTotalCapacity(c.allocator(), @intCast(cycle_slice.len + 1)));
1865+
for (cycle_slice) |prev_tracker| {
1866+
cycle_set.putAssumeCapacity(prev_tracker, {});
1867+
}
1868+
}
1869+
1870+
if (cycle_set.contains(tracker)) {
18511871
result = .{ .kind = .cycle };
18521872
break :loop;
18531873
}
@@ -1861,6 +1881,11 @@ pub const LinkerContext = struct {
18611881
const prev_source_index = tracker.source_index.get();
18621882
bun.handleOom(c.cycle_detector.append(tracker));
18631883

1884+
// If we're using the hash set, add the tracker to it as well
1885+
if (cycle_set.count() > 0) {
1886+
bun.handleOom(cycle_set.put(c.allocator(), tracker, {}));
1887+
}
1888+
18641889
// Resolve the import by one step
18651890
const advanced = c.advanceImportTracker(&tracker);
18661891
const next_tracker = advanced.value;
@@ -2730,3 +2755,19 @@ const logPartDependencyTree = bundler.logPartDependencyTree;
27302755

27312756
const jsc = bun.jsc;
27322757
const EventLoop = bun.jsc.AnyEventLoop;
2758+
2759+
/// Hash context for ImportTracker used in cycle detection
2760+
const ImportTrackerHashCtx = struct {
2761+
pub fn hash(_: @This(), tracker: ImportTracker) u64 {
2762+
// Combine source_index (u32) and import_ref hash (u64)
2763+
// name_loc is not included in the hash as it doesn't affect semantic equality
2764+
const source_hash = @as(u64, tracker.source_index.get());
2765+
const ref_hash = tracker.import_ref.hash64();
2766+
return source_hash ^ ref_hash;
2767+
}
2768+
2769+
pub fn eql(_: @This(), a: ImportTracker, b: ImportTracker) bool {
2770+
return a.source_index.get() == b.source_index.get() and
2771+
a.import_ref.eql(b.import_ref);
2772+
}
2773+
};

‎test/bundler/bundler_cycle_detection.test.ts‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import { itBundled } from "../expectBundled";
2+
3+
describe("bundler", () => {
4+
// Test basic cycle detection (uses array scan path, < 8 depth)
5+
itBundled("bundler/ShallowCycleDetection", {
6+
files: {
7+
"/entry.js": /* js */ `
8+
export {a as b} from './entry'
9+
export {b as c} from './entry'
10+
export {c as d} from './entry'
11+
export {d as a} from './entry'
12+
`,
13+
},
14+
bundleErrors: {
15+
"/entry.js": [
16+
`Detected cycle while resolving import "a"`,
17+
`Detected cycle while resolving import "b"`,
18+
`Detected cycle while resolving import "c"`,
19+
`Detected cycle while resolving import "d"`,
20+
],
21+
},
22+
});
23+
24+
// Test deep cycle detection (should use hash set path, >= 8 depth)
25+
itBundled("bundler/DeepCycleDetection", {
26+
files: {
27+
"/entry.js": /* js */ `
28+
export {a as b} from './entry'
29+
export {b as c} from './entry'
30+
export {c as d} from './entry'
31+
export {d as e} from './entry'
32+
export {e as f} from './entry'
33+
export {f as g} from './entry'
34+
export {g as h} from './entry'
35+
export {h as i} from './entry'
36+
export {i as j} from './entry'
37+
export {j as k} from './entry'
38+
export {k as a} from './entry'
39+
`,
40+
},
41+
bundleErrors: {
42+
"/entry.js": [
43+
`Detected cycle while resolving import "a"`,
44+
`Detected cycle while resolving import "b"`,
45+
`Detected cycle while resolving import "c"`,
46+
`Detected cycle while resolving import "d"`,
47+
`Detected cycle while resolving import "e"`,
48+
`Detected cycle while resolving import "f"`,
49+
`Detected cycle while resolving import "g"`,
50+
`Detected cycle while resolving import "h"`,
51+
`Detected cycle while resolving import "i"`,
52+
`Detected cycle while resolving import "j"`,
53+
`Detected cycle while resolving import "k"`,
54+
],
55+
},
56+
});
57+
58+
// Test deep chain without cycle (should work correctly)
59+
itBundled("bundler/DeepChainNoCycle", {
60+
files: {
61+
"/entry.js": /* js */ `
62+
import { x } from './a.js';
63+
console.log(x);
64+
`,
65+
"/a.js": /* js */ `
66+
import { x } from './b.js';
67+
export { x };
68+
`,
69+
"/b.js": /* js */ `
70+
import { x } from './c.js';
71+
export { x };
72+
`,
73+
"/c.js": /* js */ `
74+
import { x } from './d.js';
75+
export { x };
76+
`,
77+
"/d.js": /* js */ `
78+
import { x } from './e.js';
79+
export { x };
80+
`,
81+
"/e.js": /* js */ `
82+
import { x } from './f.js';
83+
export { x };
84+
`,
85+
"/f.js": /* js */ `
86+
import { x } from './g.js';
87+
export { x };
88+
`,
89+
"/g.js": /* js */ `
90+
import { x } from './h.js';
91+
export { x };
92+
`,
93+
"/h.js": /* js */ `
94+
export const x = 42;
95+
`,
96+
},
97+
run: {
98+
stdout: "42",
99+
},
100+
});
101+
102+
// Test cross-file cycle detection with deep chain
103+
itBundled("bundler/DeepCrossFileCycle", {
104+
files: {
105+
"/entry.js": /* js */ `
106+
export {a as b} from './foo1'
107+
export {b as c} from './foo1'
108+
`,
109+
"/foo1.js": /* js */ `
110+
export {c as d} from './foo2'
111+
export {d as e} from './foo2'
112+
`,
113+
"/foo2.js": /* js */ `
114+
export {e as f} from './foo3'
115+
export {f as g} from './foo3'
116+
`,
117+
"/foo3.js": /* js */ `
118+
export {g as h} from './foo4'
119+
export {h as i} from './foo4'
120+
`,
121+
"/foo4.js": /* js */ `
122+
export {i as j} from './entry'
123+
export {j as a} from './entry'
124+
`,
125+
},
126+
bundleErrors: {
127+
"/entry.js": [`Detected cycle while resolving import "a"`, `Detected cycle while resolving import "b"`],
128+
},
129+
});
130+
});

0 commit comments

Comments
 (0)