Repository navigation
perf(glob): track active component indices as a set to eliminate double-visits - #28496
Conversation
At a `**/X` boundary the walker has two valid interpretations: advance
past X, or keep the outer `**` alive (in case X appears again deeper).
The previous implementation pushed two WorkItems for the same directory,
each triggering a separate openat + readdir + close. The overhead
cascaded to every descendant.
Instead, carry a set of active component indices per WorkItem (an NFA
state set) and evaluate each directory entry against all active indices
in one readdir. The child's set is the union of recursion targets. When
the component after a `**/X` boundary is itself `**`, the outer
globstar is dropped entirely since the inner one's recursion subsumes it.
ComponentSet is an alias for bun.bit_set.AutoBitSet, which stores up to
127 indices inline in a [2]usize and spills to heap beyond, so patterns
of any depth remain correct. Added count/findFirstSet/iterator dispatch
methods to AutoBitSet.
Benchmark on next.js (`**/node_modules/**/*.js`, dot:true, 68K matches):
before: 2.962 s
after: 1.305 s (2.27x)
Synthetic trees with deeper nesting show up to 2.9x. Patterns without a
`**/X` boundary (e.g. `**/*.ts`) are unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughReworked glob component tracking to use a bitset-based ComponentSet for multiple active pattern indices. Added iterator and query APIs to AutoBitSet. Updated GlobWalker traversal and matching to operate over ComponentSet masks. Added a test exercising deep patterns (130 components) across several glob variants. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/glob/GlobWalker.zig`:
- Around line 1404-1411: The call to collapseSuccessiveDoubleWildcards uses
`@constCast` because collapseSuccessiveDoubleWildcards currently takes a mutable
GlobWalker pointer; change collapseSuccessiveDoubleWildcards to accept *const
GlobWalker so it can be called from normalizeIdx without casting, update its
function signature and any other callers to pass *const where appropriate, and
remove the `@constCast` in normalizeIdx (reference functions: normalizeIdx and
collapseSuccessiveDoubleWildcards).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b92c39e-cfba-4db5-82ed-7484413b811e
📒 Files selected for processing (3)
src/collections/bit_set.zigsrc/glob/GlobWalker.zigtest/js/bun/glob/scan.test.ts
| inline fn normalizeIdx(this: *const GlobWalker, idx: u32) u32 { | ||
| if (idx < this.patternComponents.items.len and | ||
| this.patternComponents.items[idx].syntax_hint == .Double) | ||
| { | ||
| return @constCast(this).collapseSuccessiveDoubleWildcards(idx); | ||
| } | ||
| return idx; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider avoiding @constCast by changing function signature.
collapseSuccessiveDoubleWildcards only reads state, so it could take *const GlobWalker. This would eliminate the need for @constCast here.
♻️ Optional refactor
- fn collapseSuccessiveDoubleWildcards(this: *GlobWalker, idx: u32) u32 {
+ fn collapseSuccessiveDoubleWildcards(this: *const GlobWalker, idx: u32) u32 {Then in normalizeIdx:
- return `@constCast`(this).collapseSuccessiveDoubleWildcards(idx);
+ return this.collapseSuccessiveDoubleWildcards(idx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/glob/GlobWalker.zig` around lines 1404 - 1411, The call to
collapseSuccessiveDoubleWildcards uses `@constCast` because
collapseSuccessiveDoubleWildcards currently takes a mutable GlobWalker pointer;
change collapseSuccessiveDoubleWildcards to accept *const GlobWalker so it can
be called from normalizeIdx without casting, update its function signature and
any other callers to pass *const where appropriate, and remove the `@constCast` in
normalizeIdx (reference functions: normalizeIdx and
collapseSuccessiveDoubleWildcards).
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this is a significant NFA-simulation refactor of the core glob walker traversal — the algorithmic change is large enough to warrant a human look before merging.
Extended reasoning...
Overview
This PR touches three files: src/collections/bit_set.zig (adds count(), findFirstSet(), and iterator() dispatch methods to AutoBitSet), src/glob/GlobWalker.zig (the core change), and test/js/bun/glob/scan.test.ts (new deep-component regression test). The GlobWalker change replaces the per-WorkItem scalar idx: u32 with a ComponentSet (AutoBitSet bitmask of active NFA states), eliminating duplicate openat + readdir + close calls at **/X boundaries.
Security risks
None apparent. This is a pure performance/correctness refactor of filesystem traversal; it does not handle credentials, permissions, or untrusted input in new ways.
Level of scrutiny
High. The change implements a non-trivial NFA state-set simulation over the filesystem. Core logic in evalDir / evalFile / evalImpl replaces three previously-duplicated inline blocks, and the "drop outer ** when inner component is also **" optimization is subtle enough to warrant careful review of all **/X/** edge cases. The set is created on every directory entry visit (makeSet() allocates from the arena each time), which is fine for ≤127-component patterns (static path, no heap) but adds arena pressure for deep patterns. The test covers the heap-backed path but does not exhaustively exercise all boundary-pattern combinations.
Other factors
The PR description is thorough and the benchmarks are compelling (2.27× for **/node_modules/**/*.js). The implementation follows established NFA simulation patterns correctly as far as can be determined from static analysis. However, at ~250 changed lines in production-critical filesystem traversal code, this merits a human sign-off before landing.
- Use bun.handleOom instead of catch bun.outOfMemory() in makeSet - Skip the 130-component test on Windows where the path exceeds MAX_PATH (260). The AutoBitSet heap path is still covered on other platforms.
There was a problem hiding this comment.
No bugs found, but this is a substantial algorithmic refactoring of the core glob walker that implements NFA state-set simulation — it warrants a human look before merging.
Extended reasoning...
Overview
This PR replaces the single component_idx integer in WorkItem with a ComponentSet (an AutoBitSet) to track multiple active NFA states simultaneously during directory traversal. Changes span src/collections/bit_set.zig (new count, findFirstSet, iterator methods on AutoBitSet), src/glob/GlobWalker.zig (major structural refactor with new evalDir/evalFile/evalImpl/makeSet/singleSet/normalizeIdx helpers), and a new 130-component test.
Security risks
None. This touches only glob pattern matching and filesystem traversal logic — no auth, crypto, or permission-sensitive code paths.
Level of scrutiny
High. The glob walker is production-critical (used by Bun.Glob.scan()) and the change is a non-trivial algorithmic refactoring. The NFA state-set simulation has subtle correctness requirements: e.g., the bump == 2 path in evalDir relies on the invariant that idx + 2 < len (which holds because matchPatternDir only returns 2 when component_idx + 1 != len - 1), and the decision to drop the outer ** when idx+2 is itself ** needs careful validation. These are correct per static analysis, but given the complexity, a human should verify the full NFA simulation logic.
Other factors
The PR is well-described with benchmarks (2.27× speedup on real-world data), the existing fast-glob e2e test suite provides broad coverage of pattern semantics, and the new 130-component test covers the heap-backed AutoBitSet path. The only reviewer comment is a trivial @constCast style nitpick from CodeRabbit. The change is net +35 lines and cleanly structured.
There was a problem hiding this comment.
No bugs found, but this is a substantial algorithmic refactor of the glob walker’s core traversal loop that warrants a human look before merging.
Extended reasoning...
Overview
This PR changes GlobWalker.zig to track multiple active pattern-component indices per WorkItem using an AutoBitSet (ComponentSet), replacing the previous single component_idx scalar. It also adds count(), findFirstSet(), and iterator() to AutoBitSet in bit_set.zig, and adds a 130-component depth test to scan.test.ts.
Security risks
None. This is a pure performance optimization with no auth, crypto, or permission-handling changes.
Level of scrutiny
High. The change rewrites the inner loop of Bun.Glob.scan() — the .file, .directory, .sym_link, and .unknown branches all switch from inline single-index logic to evalFile/evalDir/evalImpl helpers that iterate a bitset. The evalDir boundary condition (if (bump == 2 and comps[idx + 2].syntax_hint != .Double)) is a subtle invariant that depends on matchPatternDir never returning 2 when idx + 2 >= len — true by inspection but easy to regress. The normalizeIdx helper and its @constCast workaround add further complexity.
Other factors
The algorithm (NFA state-set simulation) is correct in principle and the benchmarks are convincing. The test covers the heap-backed AutoBitSet path. No bugs were flagged by the automated hunting system. The only reviewer comment is a trivial @constCast nitpick. Given the breadth of the traversal code touched, a human reviewer familiar with the glob walker should sign off.
PR #28496 replaced the single component_idx with an active BitSet, but the Windows NtQueryDirectoryFile filter call from PR #28489 still referenced the old variable name. computeNtFilter operates on a single pattern component, so apply the kernel filter only when exactly one index is active. For multi-index states (e.g. after **), skip the filter to avoid hiding entries needed by other indices. The filter is a pre-filter optimization; matchPatternImpl still runs for correctness.
PR #28496 replaced the single component_idx with an active BitSet, but the Windows NtQueryDirectoryFile filter call from PR #28489 still referenced the old variable name. computeNtFilter operates on a single pattern component, so apply the kernel filter only when exactly one index is active. For multi-index states (e.g. after **), skip the filter to avoid hiding entries needed by other indices. The filter is a pre-filter optimization; matchPatternImpl still runs for correctness.
## What Fixes a build error on `main` introduced by the interaction of #28496 and #28489: ``` src/glob/GlobWalker.zig:715:69: error: use of undeclared identifier 'component_idx' iterator.setNameFilter(this.computeNtFilter(component_idx)); ^~~~~~~~~~~~~ ``` ## Why \#28496 replaced the single `component_idx` variable with an `active` BitSet to track multiple active pattern component indices. \#28489 (merged just before) added a Windows `NtQueryDirectoryFile` filter call that references `component_idx`. After both landed, the variable no longer exists in scope. ## Fix `computeNtFilter` operates on a single pattern component, so it can only be applied when exactly one index is active. For multi-index states (e.g. after `**`), a single-component kernel filter could hide entries that other active indices need to match, so we skip it. This is safe because the NT filter is purely a pre-filter optimization — `matchPatternImpl` still runs on every returned entry for correctness (per the existing doc comment on `computeNtFilter`). ## Verification `bun run zig:check-all` passes on all platforms (macOS/Linux/Windows × x86_64/aarch64 × Debug/Release).
…le-visits (oven-sh#28496) ## What Eliminates redundant directory visits in `Bun.Glob.scan()` for patterns containing `**/X/...`. Directories under the boundary were being opened and read twice; they're now read once. ## Why A glob pattern like `**/node_modules/**/*.js` is split into components: idx: 0 1 2 3 ** node_modules ** *.js When the walker is at `**` (idx 0) and encounters a directory named `node_modules`, there are two valid interpretations: - **Advance**: this is the `node_modules` we were looking for — jump to idx 2 and match `*.js` inside. - **Keep going**: `**` matches zero or more dirs, so `node_modules` might just be another directory under `**`. Stay at idx 0 in case there's another `node_modules` deeper. Both are correct and both can produce matches. The previous implementation handled this by pushing two `WorkItem`s for the same path — one at idx 0, one at idx 2. Each triggered its own `openat + readdir + close`, and since both recurse, every descendant of the boundary was read twice. ## How Instead of forking the traversal, carry a **set** of active component indices per `WorkItem`. When a boundary is crossed, both indices go into one set; the directory is iterated once, and each entry is checked against all active positions. The child's set is the union of what each active index says to do next. Additionally, when the component after a `**/X` boundary is itself `**` (as in `**/node_modules/**/*.js`), the outer `**` is dropped entirely — the inner one's recursion already covers everything the outer one would find. This is the standard NFA state-set simulation, applied to filesystem traversal. ## Benchmarks `**/node_modules/**/*.js` with `{ dot: true }` on the next.js repo (68,233 matches, macOS arm64): <img width="1664" height="537" alt="Screenshot 2026-03-23 at 10 36 43 PM" src="https://github.com/user-attachments/assets/57ffb5ba-0f4f-4536-9339-fc38f669d4cf" /> Gain scales with how much of the tree is under the `**/X` boundary. Synthetic trees with deeper nesting hit 2.9×. Patterns without a boundary (`**/*.ts`) are unchanged. ## Implementation - `ComponentSet` is an alias for `bun.bit_set.AutoBitSet`, which stores up to 127 indices inline in `[2]usize` and spills to heap beyond — patterns of any depth stay correct. - Added `count()`, `findFirstSet()`, and `iterator()` dispatch methods to `AutoBitSet`. - `evalDir`/`evalFile`/`evalImpl` replace the inline per-index logic; they iterate the active set and accumulate results. - Net +35 lines (the set helpers collapsed triplicated bump-handling blocks in the old code). - Added a test with 130-component patterns to cover the heap-backed path.
…#28543) ## What Fixes a build error on `main` introduced by the interaction of oven-sh#28496 and oven-sh#28489: ``` src/glob/GlobWalker.zig:715:69: error: use of undeclared identifier 'component_idx' iterator.setNameFilter(this.computeNtFilter(component_idx)); ^~~~~~~~~~~~~ ``` ## Why \oven-sh#28496 replaced the single `component_idx` variable with an `active` BitSet to track multiple active pattern component indices. \oven-sh#28489 (merged just before) added a Windows `NtQueryDirectoryFile` filter call that references `component_idx`. After both landed, the variable no longer exists in scope. ## Fix `computeNtFilter` operates on a single pattern component, so it can only be applied when exactly one index is active. For multi-index states (e.g. after `**`), a single-component kernel filter could hide entries that other active indices need to match, so we skip it. This is safe because the NT filter is purely a pre-filter optimization — `matchPatternImpl` still runs on every returned entry for correctness (per the existing doc comment on `computeNtFilter`). ## Verification `bun run zig:check-all` passes on all platforms (macOS/Linux/Windows × x86_64/aarch64 × Debug/Release).
…le-visits (oven-sh#28496) ## What Eliminates redundant directory visits in `Bun.Glob.scan()` for patterns containing `**/X/...`. Directories under the boundary were being opened and read twice; they're now read once. ## Why A glob pattern like `**/node_modules/**/*.js` is split into components: idx: 0 1 2 3 ** node_modules ** *.js When the walker is at `**` (idx 0) and encounters a directory named `node_modules`, there are two valid interpretations: - **Advance**: this is the `node_modules` we were looking for — jump to idx 2 and match `*.js` inside. - **Keep going**: `**` matches zero or more dirs, so `node_modules` might just be another directory under `**`. Stay at idx 0 in case there's another `node_modules` deeper. Both are correct and both can produce matches. The previous implementation handled this by pushing two `WorkItem`s for the same path — one at idx 0, one at idx 2. Each triggered its own `openat + readdir + close`, and since both recurse, every descendant of the boundary was read twice. ## How Instead of forking the traversal, carry a **set** of active component indices per `WorkItem`. When a boundary is crossed, both indices go into one set; the directory is iterated once, and each entry is checked against all active positions. The child's set is the union of what each active index says to do next. Additionally, when the component after a `**/X` boundary is itself `**` (as in `**/node_modules/**/*.js`), the outer `**` is dropped entirely — the inner one's recursion already covers everything the outer one would find. This is the standard NFA state-set simulation, applied to filesystem traversal. ## Benchmarks `**/node_modules/**/*.js` with `{ dot: true }` on the next.js repo (68,233 matches, macOS arm64): <img width="1664" height="537" alt="Screenshot 2026-03-23 at 10 36 43 PM" src="https://github.com/user-attachments/assets/57ffb5ba-0f4f-4536-9339-fc38f669d4cf" /> Gain scales with how much of the tree is under the `**/X` boundary. Synthetic trees with deeper nesting hit 2.9×. Patterns without a boundary (`**/*.ts`) are unchanged. ## Implementation - `ComponentSet` is an alias for `bun.bit_set.AutoBitSet`, which stores up to 127 indices inline in `[2]usize` and spills to heap beyond — patterns of any depth stay correct. - Added `count()`, `findFirstSet()`, and `iterator()` dispatch methods to `AutoBitSet`. - `evalDir`/`evalFile`/`evalImpl` replace the inline per-index logic; they iterate the active set and accumulate results. - Net +35 lines (the set helpers collapsed triplicated bump-handling blocks in the old code). - Added a test with 130-component patterns to cover the heap-backed path.
…#28543) ## What Fixes a build error on `main` introduced by the interaction of oven-sh#28496 and oven-sh#28489: ``` src/glob/GlobWalker.zig:715:69: error: use of undeclared identifier 'component_idx' iterator.setNameFilter(this.computeNtFilter(component_idx)); ^~~~~~~~~~~~~ ``` ## Why \oven-sh#28496 replaced the single `component_idx` variable with an `active` BitSet to track multiple active pattern component indices. \oven-sh#28489 (merged just before) added a Windows `NtQueryDirectoryFile` filter call that references `component_idx`. After both landed, the variable no longer exists in scope. ## Fix `computeNtFilter` operates on a single pattern component, so it can only be applied when exactly one index is active. For multi-index states (e.g. after `**`), a single-component kernel filter could hide entries that other active indices need to match, so we skip it. This is safe because the NT filter is purely a pre-filter optimization — `matchPatternImpl` still runs on every returned entry for correctness (per the existing doc comment on `computeNtFilter`). ## Verification `bun run zig:check-all` passes on all platforms (macOS/Linux/Windows × x86_64/aarch64 × Debug/Release).
3 participants
What
Eliminates redundant directory visits in
Bun.Glob.scan()for patternscontaining
**/X/.... Directories under the boundary were being openedand read twice; they're now read once.
Why
A glob pattern like
**/node_modules/**/*.jsis split into components:When the walker is at
**(idx 0) and encounters a directory namednode_modules, there are two valid interpretations:node_moduleswe were looking for — jump toidx 2 and match
*.jsinside.**matches zero or more dirs, sonode_modulesmightjust be another directory under
**. Stay at idx 0 in case there'sanother
node_modulesdeeper.Both are correct and both can produce matches. The previous
implementation handled this by pushing two
WorkItems for the samepath — one at idx 0, one at idx 2. Each triggered its own
openat + readdir + close, and since both recurse, every descendant ofthe boundary was read twice.
How
Instead of forking the traversal, carry a set of active component
indices per
WorkItem. When a boundary is crossed, both indices go intoone set; the directory is iterated once, and each entry is checked
against all active positions. The child's set is the union of what each
active index says to do next.
Additionally, when the component after a
**/Xboundary is itself**(as in
**/node_modules/**/*.js), the outer**is dropped entirely —the inner one's recursion already covers everything the outer one would
find.
This is the standard NFA state-set simulation, applied to filesystem
traversal.
Benchmarks
**/node_modules/**/*.jswith{ dot: true }on the next.js repo(68,233 matches, macOS arm64):
Gain scales with how much of the tree is under the
**/Xboundary.Synthetic trees with deeper nesting hit 2.9×. Patterns without a
boundary (
**/*.ts) are unchanged.Implementation
ComponentSetis an alias forbun.bit_set.AutoBitSet, which storesup to 127 indices inline in
[2]usizeand spills to heap beyond —patterns of any depth stay correct.
count(),findFirstSet(), anditerator()dispatch methodsto
AutoBitSet.evalDir/evalFile/evalImplreplace the inline per-index logic;they iterate the active set and accumulate results.
blocks in the old code).
path.