fix: gitignoringGlob stack safety + perf #1234
Merged
Conversation
This file contains 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
cakekindel
commented
Jun 17, 2024
| @@ -66,3 +73,9 @@ spec = Spec.around globTmpDir do | |||
| FS.writeTextFile (Path.concat [p, ".gitignore"]) """/fruits\n/src""" | |||
| a <- Glob.gitignoringGlob p ["fruits/apple/**"] | |||
| a `Assert.shouldEqual` ["fruits/apple"] | |||
|
|
|||
| Spec.it "is stacksafe" \p -> do | |||
There was a problem hiding this comment.
fails on f0f7901 (tip of master at the moment)
cakekindel
commented
Jun 17, 2024
src/Spago/Glob.purs
Outdated
| import Effect.Aff as Aff | ||
| import Effect.Ref as Ref | ||
| import Node.FS.Sync as SyncFS | ||
| import Node.Path as Path | ||
| import Record as Record | ||
| import Type.Proxy (Proxy(..)) | ||
|
|
||
| type MicroMatchOptions = { ignore :: Array String } | ||
| type MicroMatchOptions = { ignore :: Array String, include :: Array String } |
There was a problem hiding this comment.
this change allows us to accumulate with appending arrays instead of ||ing functions.
Fun fact for those who may not be aware, if all of a record's fields are Semigroup, the record is Semigroup so you can <> these :)
|
is micromatch not cross-platform? 💢 |
|
I am very confused as well - I went through the code and all the paths seem to be handled properly, so I am not sure why the failures |
|
I'd imagine it's because there's an assumption that |
f-f
approved these changes
Jun 24, 2024
3 tasks
f-f
pushed a commit
that referenced
this pull request
Jun 24, 2024
This was referenced Jul 3, 2024
f-f
added a commit
that referenced
this pull request
Jul 5, 2024
The final patch goes back to the approach we had before #1234, where we'd compose the glob-matching functions instead of keeping a list of ignores, and recompute the matchers when needed. The patch in #1234 was not optimised, as the matchers were being recreated for every file encountered, and optimising that in the first two commits of this PR got us down to 2x of the performance pre-1234. That is unfortunately still not acceptable, so I reintroduced the function-composition approach, which is still prone to blowing the stack - the change here should reduce that risk, since instead of composing every line of gitignores as a separate matcher in the chain, we instead nest them in a single or block. That should dramatically reduce the size of the call chain.
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.
Description of the change
fixes #1231
Before, we were eagerly turning string glob patterns to matching functions
String -> Boolean, and adding new patterns with||. The instance(HeytingAlgebra b) => HeytingAlgebra (a -> b)isn't stack safe, causing sufficiently large gitignore files to overflow the call stack.Now, we accumulate the patterns themselves and build them into micromatch patterns when we evaluate them. The actual call to
micromatchcosts practically nothing, so this flat-out wins time & space cost (as well as fixing the stack-safety issue)Turns out we've learned which option is more performant 😅
Checklist:
READMEP.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊