Glob Match Rewrite by probably-neb · Pull Request #16824 · oven-sh/bun

probably-neb · 2025-01-28T02:49:57Z

What does this PR do?

Rewrites the glob match implementation (both ascii and non-ascii) by porting the fast-glob crate

This fixes #14934 as well as a few other issues I found along the way.
Additionally, based on my local testing it results in a large perf win.

The perf win I am referring to was measured by running each test in test/js/bun/glob/match.test.ts 10,000 times each and comparing the total runtime for the current and my new implementation. On my machine I get an average of a little over 200x faster than the current implementation. I'd be happy to upload the code for this comparison to github and share if it is of interest.

I didn't do very much more in depth perf testing because my main focus with the PR was to fix the aforementioned issues with the current implementation.

The high-level overview of the change is instead of keeping a track of a BraceStack to deal with braces {foo,bar}, it recursively creates new globs for each branch (i.e. 1{foo,bar}2 -> [1foo2,1bar2]).
This does mean, however, that matchImpl must now take in an allocator parameter in order to create the sub globs. For better performance the implementation basically uses a std.stackFallback to stack allocate sub globs less than a predetermined length defined by the GLOB_STACK_BUF_SIZE variable in both src/glob/ascii.zig and src/glob/GlobWalker.zig (currently 64 in both places).

You will notice in the PR that I updated all callsites of matchImpl to pass in an instance of Allocator. I would appreciate if someone more familiar with the codebase than I would double check each place that I did so to make sure I'm using the allocator that makes the most sense in each context. Please note that in places where an allocator was not available in the context I used bun.default_allocator rather than change more function signatures, but I'm happy to change this if desired.

Code changes

How did you verify your code works?

I wrote automated tests

I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
I included a test for the new code, or an existing test covers it
I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)

…brace branch buffer

probably-neb · 2025-01-28T02:51:34Z

Here is the hyperfine output for my perf test.
original: current glob match implementation
rewrite-v0: rewritten implementation before perf improvements commit
rewrite-v3: rewritten implementation after perf improvements commit

❱ hyperfine -N ./original ./rewrite-v3 ./rewrite-v0
Benchmark 1: ./original
  Time (mean ± σ):     117.691 s ±  0.910 s    [User: 38.036 s, System: 79.520 s]
  Range (min … max):   116.630 s … 119.453 s    10 runs

Benchmark 2: ./rewrite-v3
  Time (mean ± σ):     566.2 ms ±  32.4 ms    [User: 565.0 ms, System: 0.0 ms]
  Range (min … max):   540.2 ms … 619.7 ms    10 runs

Benchmark 3: ./rewrite-v0
  Time (mean ± σ):      2.753 s ±  0.087 s    [User: 0.808 s, System: 1.783 s]
  Range (min … max):    2.680 s …  2.916 s    10 runs

Summary
  ./rewrite-v3 ran
    4.86 ± 0.32 times faster than ./rewrite-v0
  207.86 ± 11.99 times faster than ./original

RiskyMH · 2025-01-28T04:11:49Z

I'm not sure if the tests are flaky, but they do seem related: https://buildkite.com/bun/bun/builds/10675

zackradisic · 2025-01-28T04:16:57Z

Thanks a lot @probably-neb, porting the fast-glob crate is something that I've been meaning to do for a while but never had the time to do so!

I'll take a closer look at this PR when I get the chance, but thanks once again!

probably-neb · 2025-01-28T04:19:39Z

@RiskyMH they do seem related. I'll look into it

probably-neb · 2025-01-28T05:06:25Z

Ok something is very wrong with the Apple x64 output. Won't show it in buildkite and when I download it it's full of null bytes. Not sure what to do about that one.
Seems like the glob ./** is failing to match any paths on the windows tests that are failing. Unfortunately my Windows install decided it is unable to connect to the internet a few weeks ago so I can't test it on Windows ATM.
I'd be happy to join a call on Discord with someone who does and help them debug it tomorrow, otherwise I will just try to get Windows working again this weekend and attempt to debug it myself

zackradisic · 2025-01-30T07:03:57Z

+        var buf: [GLOB_STACK_BUF_SIZE]u8 = undefined;
+        var fixed_buf_alloc = std.heap.FixedBufferAllocator.init(&buf);
+        const buf_alloc = fixed_buf_alloc.allocator();


These 3 lines should probably be above and happen unconditionally. Haven't verified but it looks like it will cause undefined behavior as the code is exiting the scope in which they were defined

zackradisic · 2025-02-06T23:58:24Z

Hey @probably-neb thanks once again for the PR. Bun's Glob is in need of some refactoring so I'm gonna take it from here and do some changes like removing the ascii and unicode matchers as only the ascii matcher is necessary, as well as some other changes.

There are two options:

you could give me write access to your fork so I can make the changes their and we can use this same PR
I push these changes into a branch in the Bun repo and make a PR and reference this one

Which one would you like to proceed with?

probably-neb · 2025-02-07T01:14:52Z

Thanks a bunch, I've really been meaning to get back around to this, but I've been swamped with work and unable to get Windows working again.

I'm fine with either approach. I'd love if my name was attached in the git log somewhere but I don't feel that strongly about it.

I invited you as a collaborator to my fork. Feel free to proceed with whichever approach is easier for you

probably-neb · 2025-02-07T01:22:33Z

Also feel free to ask questions if you have any. Happy to continue helping to the extent I can

Jarred-Sumner · 2025-02-13T05:49:05Z

+            var width: u32 = 0;
+            while (i < pattern.len) : (i += 1) {
+                const c = pattern[i];
+                width = bun.strings.utf8ByteSequenceLength(c);


This should use the CodepointIterator because this code will not handle invalid surrogate pairs correctly on Windows

Jarred-Sumner · 2025-02-13T05:49:28Z

+import { isWindows } from "harness";

 describe("Glob.match", () => {
+  test("WTF", () => {


Can you delete this test

Jarred-Sumner · 2025-02-13T05:50:44Z

-                else => {},
-            }
+        pub fn deinit(_: @This(), _: std.mem.Allocator) void {
+            // switch (this) {


Can you delete this function so that the next person working on this code doesn't think it's a memory leak?

Jarred-Sumner · 2025-02-13T05:51:17Z

                    else => {},
                }
            }
+            i -|= 1;


this seems likely to be incorrect?

Jarred-Sumner · 2025-02-13T05:51:36Z

@@ -0,0 +1,27 @@
+import { Glob } from "bun";


can you give this a better filename?

probably-neb added 9 commits January 27, 2025 19:56

add breaking glob match tests

9ebf70f

port https://github.com/shulaoda/fast-glob

be7723f

pass allocator to ascii glob match

f1c49df

port fast-glob to glob match unicode

75e5381

pass allocator to non-ascii glob match calls

bb25f86

perf improvements

c15ba64

inline matchBraceBranch

58fdecc

call bun.outOfMemory instead of unreachable on failure to allocate …

cece995

…brace branch buffer

add benchmark file for glob rewrite

4fb2c99

zackradisic reviewed Jan 30, 2025

View reviewed changes

Merge branch 'main' into probably-neb-probably-neb/fast-glob-port

50c4a0b

Move that out

ff60e8b

zackradisic added 10 commits February 7, 2025 18:25

wip

6e7bdb0

wip

a535ac0

yooops

6b2d26c

fix finally lol

ef01f9a

wip remove codepoint glob stuff

2ef4d7a

remove codepoint glob match

448d330

fix

07b811b

fixes

d4f3b31

remove switch if arm because they don't work in zig

9210581

use brace stack

94e05d5

zackradisic added 3 commits February 12, 2025 18:10

rename ascii.zig -> match.zig

7df29b6

Merge branch 'main' into probably-neb-probably-neb/fast-glob-port

f28b88b

stupid path shit wtf???

ba25d0c

Jarred-Sumner reviewed Feb 13, 2025

View reviewed changes

Comment thread src/glob/GlobWalker.zig

else => {},

}

}

i -|= 1;

Copy link
Copy Markdown

Collaborator

Jarred-Sumner Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems likely to be incorrect?

Jarred-Sumner reviewed Feb 13, 2025

View reviewed changes

zackradisic added 2 commits February 13, 2025 01:01

rename

18c2bb6

changes

35999a0

Jarred-Sumner merged commit b44769c into oven-sh:main Feb 14, 2025

dylan-conway mentioned this pull request May 16, 2025

Bun.Glob.scan and Bun.Glob.match behaviour mismatch #12120

Closed

coderabbitai Bot mentioned this pull request Mar 25, 2026

fix: replace npm glob/minimatch with Bun.Glob — fixes 52 CI test failures AltimateAI/altimate-code#460

Merged

3 tasks

Conversation

probably-neb commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

How did you verify your code works?

Uh oh!

probably-neb commented Jan 28, 2025

Uh oh!

RiskyMH commented Jan 28, 2025

Uh oh!

zackradisic commented Jan 28, 2025

Uh oh!

probably-neb commented Jan 28, 2025

Uh oh!

probably-neb commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zackradisic Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

zackradisic commented Feb 6, 2025

Uh oh!

probably-neb commented Feb 7, 2025

Uh oh!

probably-neb commented Feb 7, 2025

Uh oh!

Jarred-Sumner Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

Jarred-Sumner Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

Jarred-Sumner Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

Jarred-Sumner Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

Jarred-Sumner Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

probably-neb commented Jan 28, 2025 •

edited

Loading

probably-neb commented Jan 28, 2025 •

edited

Loading