perf: use a custom trie for path conflict validation#302
Conversation
letFunny
commented
Jun 10, 2026
- Have you signed the CLA?
d4113ef to
b8da2e8
Compare
b8da2e8 to
2191ba1
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces a trie-based conflict detector for slice path collisions and expands tests to cover more glob/double-glob conflict cases.
Changes:
- Replaced the O(n²) glob/generate conflict scan in
Release.validate()with apathConflictTree-based detector. - Added extensive regression tests for glob conflicts and new focused unit tests for the conflict tree/segmenter.
- Exposed internal conflict-tree types/helpers to external tests via
export_test.go.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/setup/setup.go | Switches validation to the new conflict-tree implementation. |
| internal/setup/conflict.go | Adds trie-based conflict detection and path segmentation logic. |
| internal/setup/setup_test.go | Adds multiple new test cases for glob/double-glob conflicts. |
| internal/setup/conflict_test.go | Adds unit tests for pathToSegments and the conflict tree structure. |
| internal/setup/export_test.go | Exports internal symbols to support setup_test package tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
After some very light work on performance to improve the algorithm without added complexity (basically caching some data to avoid map lookups, iteration order, etc. *) the performance is: *: All verified to reduce the number of cache misses using |
upils
left a comment
There was a problem hiding this comment.
Thanks @letFunny ! I am amazed at how faster this is. I even looked at the messages we display to the user to see if we could remove any as this is now pretty much instantaneous.
The overall approach looks very sane. I could not find corner cases not handled properly. I have suggestions and questions but nothing that would change the approach.
This is definitely more complex than the previous implementation but the massive performance gains justify it I think.
| // explicit termination in the trie to distinguish them from parent directories | ||
| // of other paths. | ||
| func pathToSegments(path string) ([]segment, error) { | ||
| if path[0] != '/' { |
There was a problem hiding this comment.
| if path[0] != '/' { | |
| if path == "" || path[0] != '/' { |
otherwise path[0] panics
There was a problem hiding this comment.
i guess the error message needs adjusting or there should be two checks with different error messages then
There was a problem hiding this comment.
There was a problem hiding this comment.
yep. the legitimate case i found was my own validation of this PR for which i wrote some fuzzers, and just directly called the fun. can be easily added if we ever add fuzzers here.
for reference, my fuzzers
all clean on 5m runs
There was a problem hiding this comment.
Ty for looking into this
There was a problem hiding this comment.
i did some fuzzing to validate this PR and all looks green on 5m run. +1 from me :)
for reference, my fuzzers
|
@letFunny FYI found some more speed (and memory) letFunny#29. i promise i'm going to stop tinkering now. 😅 |
