fs: cache glob matcher functions in path.matchesGlob#63915
Open
beeequeue wants to merge 2 commits into
Open
Conversation
Signed-off-by: Adam Haglund <adam@haglund.dev>
Signed-off-by: Adam Haglund <adam@haglund.dev>
Collaborator
|
Review requested:
|
beeequeue
commented
Jun 14, 2026
| nocaseMagicOnly: true, | ||
| }); | ||
|
|
||
| patternFnCache ??= new SafeMap(); |
Author
There was a problem hiding this comment.
i'm not sure if it's worth it to delay the map instantiation like this, but did it just in case.
| }); | ||
| patternFnCache.set(pattern, matcher); | ||
|
|
||
| if (patternFnCache.size >= 250) { |
Author
There was a problem hiding this comment.
randomly picked number, i'm not sure how to find a "good" value though.
| if (patternFnCache.has(pattern)) { | ||
| matcher = patternFnCache.get(pattern); | ||
| } else { | ||
| matcher = createMatcher(pattern, { |
Author
There was a problem hiding this comment.
i re-used createMatcher from above as i saw that it uses the same default options as the previous lines did.
i thought about putting the caching in that function instead, but then we would need to base the caching on all the options it accepts, so i decided to keep it here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this PR adds benchmarks for
path.matchesGlob()as well as a simple cache for pattern matching functions.rationale
most glob matching libraries in the ecosystem (including
minimatch) provide an API to let users re-use matchers instead of re-creating them every call.node doesn't provide such an API, and does not re-use matchers when possible, leading to it being much slower (10-15x) than just using
new Minimatch()directly when matching a pattern more than once.implementation details
i decided to "hide" away the cache inside the function rather than add a new API as most users will expect the function to not be a potential footgun, and the pattern has been used before in zeptomatch
since
matchGlobPattern()doesn't accept any options other than pattern and path we can safely cache it by the pattern string. if this changes in the future the cache would need to accomodate the new options in the cache keys.i went with the simplest
Mapcache implementation i could come up with as i couldn't find any LRU implementations anywhere in the codebase, if you have any suggestions for improvements please let me know!testing
i'm not sure how to test this properly, e.g. if i should export the cache instance and check its size, etc
some guidance here is very appreciated :)
benchmark results
ran on my windows 11 machine with an AMD Ryzen 9800X3D
note: these benchmarks do not test the case where we evict keys from the cache
i also have a personal benchmarking repo where i found this issue, where the results now look like this:
screenshot