fs: implement glob natively - #65392
Conversation
|
Review requested:
|
|
cc @isaacs @nodejs/fs |
f172fce to
8bedc81
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #65392 +/- ##
==========================================
- Coverage 90.19% 90.01% -0.18%
==========================================
Files 770 783 +13
Lines 264410 267019 +2609
Branches 50243 50835 +592
==========================================
+ Hits 238490 240368 +1878
- Misses 16924 17281 +357
- Partials 8996 9370 +374
🚀 New features to boost your workflow:
|
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:
I also think this can bring some improvements to things like the test runner. For instance, |
8bedc81 to
55e4ca3
Compare
|
(Whoops!) |
|
Here's the results of Notes:
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. |
fd617c2 to
77675d2
Compare
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 |
c7f32c9 to
721c2e2
Compare
|
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. |
772c5a5 to
8bca95e
Compare
8bca95e to
fbbb04c
Compare
|
lol right now I use fd in child process and it's still much faster than nodejs native glob. I hope this is fatwr than fd, if not, make it |
|
current benchmark results running on my local 88-core tower: |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
cc @nodejs/build, |
fbbb04c to
ca1176d
Compare
ca1176d to
6ff6ff1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: avivkeller <me@aviv.sh>
6ff6ff1 to
ba3353a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
gurgunday
left a comment
There was a problem hiding this comment.
Nice work! I've been playing with the implementation this morning and it lgtm overall
I just have one question/concern
| if (request->inline_only_) { | ||
| request->DoThreadPoolWork(); | ||
| request->AfterThreadPoolWork(0); | ||
| return; | ||
| } |
There was a problem hiding this comment.
So inline_only_ is curious here:
inline_only_(env->permission()->enabled() || filter_ != nullptr)
If there is an exclude function for instance or permission model is enabled, we get into this path, which calls DoThreadPoolWork, and on_main_thread is true
Therefore, will the following code not block the event loop even if it's using the async implementation?
fs.glob('**/*.cc', {
cwd: 'deps/v8',
exclude: () => false,
}, callback);
console.log('returned'); // Blocked immediately because traversal is syncFor compatibility with main, the traversal itself should not be blocking in the given example
Ref: https://openjs-foundation.slack.com/archives/C019Y2T6STH/p1787069174254509
(AI assistance used to help port minimatch to C++ and take improvements from other implementations, e.g. Rust's fast-glob [oxc] and picomatch, etc).
I'd love some pretty pedantic reviews since this is substantial. It's 1:1 with the existing glob, so it's non-breaking, but please tear this apart just in case. This implementation is over 2x faster than the old one.
Benchmarking Result
AI-Assisted-By: Claude Fable 5, Claude Opus 5