Review requested:

• @nodejs/actions
• @nodejs/performance
• @nodejs/security-wg
• @nodejs/tsc

cc @isaacs @nodejs/fs

Codecov Report

❌ Patch coverage is 76.20199% with 886 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.92%. Comparing base (ce40aa2) to head (772c5a5).
⚠️ Report is 3 commits behind head on main.

@@            Coverage Diff             @@
##             main   #65392      +/-   ##
==========================================
- Coverage   90.14%   89.92%   -0.22%     
==========================================
  Files         751      764      +13     
  Lines      253635   256475    +2840     
  Branches    47793    48440     +647     
==========================================
+ Hits       228632   230636    +2004     
- Misses      16249    16706     +457     
- Partials     8754     9133     +379     

... and 37 files with indirect coverage changes

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

take improvements from other implementations, e.g. Rust's fast-glob [oxc] and picomatch, etc

I would feel more comfortable adopting one of those instead of having to maintain our own, have you explored that route?

I would feel more comfortable adopting one of those instead of having to maintain our own, have you explored that route?

I have, but the perf wins here are dependent on this being in native, and not in JS:

1. (Minor) We are calling libuv directly, so we don't need to flip-flop between C++ and JS

2. (Major) We are writing code highly optimized for Node.js's usage. Existing JS implementations compile to a RegEx and rely on V8 to optimize it, however, we can skip that whole step by not relying on V8 at all, and manually compiling the glob to an AST.

   If a directory has 100k entries and 500 match, existing implementations are forced to create and discard ~99.5k strings to get there (since they are written in JS), in a native implementation, we don't need to create, allocate, or garbage collect anything inside of V8, we can handle it all natively.

I also think this can bring some improvements to things like the test runner. For instance, --test-coverage-include/exclude is globbed on the native side (like a supercharged version of #65371), we should be able to bring this performance improvement elsewhere. Maybe this can help in similar ways in other places to?

(Whoops!)

Here's the results of node-glob's benchmark script with this branch, as of fd617c2: https://gist.github.com/isaacs/abf2714edd59cb6803d36ff6b3407d22

Notes:

1. Correctness does not seem to be impacted in any way. That's great! Agents often take shortcuts in tasks like this, so it's good to see that the tests kept it well behaved ;)
2. The fs.globSync performance is a bit faster than node-glob. That's not unexpected, since it was already comparable, but it does seem to have improved.
3. The fs.glob performance (async) is still considerably slower. The only explanation I can imagine for this is that it's still stacking waits serially rather than parallelizing, or the more aggressive caching with path-scurry is saving node-glob from a lot of repetitive computations. (I know the node impl does cache, but it might not be caching as much or as aggressively; path-scurry effectively caches every path and fs operation for the duration of the walk.)
4. Performance of fast-glob and globby are there for scale, but they can be safely ignored; these implementations sacrifice correctness for speed, and thus are not apples-to-apples comparable.

Given the deep cuts made here, I'd expect to see the native implementation get at least 5 to 10 times faster than node-glob. No longer being 6x slower is indeed a significant improvement, perhaps enough to make it worth landing this as-is and making further improvements later. But it's definitely still not what I'd call "done", by quite a bit.

I have, but the perf wins here are dependent on this being in native, and not in JS:

It's not what I meant though, my question is whether you explored adopting one of the native alternatives your AI based itself on (you mentioned Rust, surely that is not JS)

It's not what I meant though, my question is whether you explored adopting one of the native alternatives your AI based itself on (you mentioned Rust, surely that is not JS)

https://github.com/oxc-project/fast-glob, the Rust implementation I mentioned, isn't compliant with minimatch (e.g. negated patterns don't work). A lot of the really fast implementations aren't compatible with our current set up (as they match different globs). As @isaacs said about a different fast-glob (a different library of the same name), "these implementations sacrifice correctness for speed, and thus are not apples-to-apples comparable." That can be said about several implementations, so working from scratch was the best way to both match the existing behavior and take the performance benefits from the existing implementations.

Minimatch had its own tests that covered it extensively. Those tests have not been ported over to this codebase, which I think should be to guarantee backward-compatibility and no regressions.

Minimatch also had documentation for the glob syntax that does not appear to be preserved. The source itself could use code comments and a description of the glob syntax and rules.

I plan on taking an additional detailed review pass on this over the next day or two. I know you have two sign offs already. I'd appreciate if you let it sit for a few days for review before landing.

Of course!

ok, handful of additional comments. Main issue is that test coverage needs to be expanded. couple of bugs but overall solid.

Breaking change since a string that contains every unicode character will now throw an error (that being said, this should never occur in pretty much any case). But, it's still a change.

ok, handful of additional comments. Main issue is that test coverage needs to be expanded. couple of bugs but overall solid.

I ported fd43e85#diff-52af056e4dffb90208b74c52cfa8646e6bd486b73ddf0c71c907e1fdc52c0a51 from minimatch, and everything passes

CI: https://ci.nodejs.org/job/node-test-pull-request/76272/

Does our CI still not like merges (should I rebase this?), or have things changed in the recent past?

@avivkeller ... always rebase, never merge.