-
Notifications
You must be signed in to change notification settings - Fork 57
perf: improve release validation performance #279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ package setup | |||||||
| import ( | ||||||||
| "errors" | ||||||||
| "fmt" | ||||||||
| "maps" | ||||||||
| "os" | ||||||||
| "path/filepath" | ||||||||
| "slices" | ||||||||
|
|
@@ -273,14 +274,47 @@ func (r *Release) validate() error { | |||||||
| } | ||||||||
|
|
||||||||
| // Check for glob and generate conflicts. | ||||||||
| // | ||||||||
| // This is the most time-consuming part of every command because checking | ||||||||
| // each pair of paths uses an n^2 algorithm; specifically due to glob | ||||||||
| // handling. | ||||||||
| // | ||||||||
| // We can speed up the search by looking at the prefixes of each path that | ||||||||
| // don't contain globs. Specifically two paths a and b conflict if and only | ||||||||
| // if they both start with the prefix of a or with the prefix of b. We can | ||||||||
| // discard directly all other parts that do not share either of the | ||||||||
| // prefixes. This can be done efficiently with a radix tree or a trie or a | ||||||||
| // simple list of sorted paths and using binary search (see below). | ||||||||
|
Comment on lines
+286
to
+287
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is outlining several strategies but the code is only using of them. |
||||||||
| // | ||||||||
| // Note: to check both prefixes we have to check `(a,b)` and `(b,a)` as the | ||||||||
| // code below is not symmetric. | ||||||||
| allPaths := slices.Collect(maps.Keys(paths)) | ||||||||
| slices.Sort(allPaths) | ||||||||
|
Comment on lines
+291
to
+292
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| for oldPath, oldSlices := range paths { | ||||||||
| for _, old := range oldSlices { | ||||||||
| oldInfo := old.Contents[oldPath] | ||||||||
| if oldInfo.Kind != GeneratePath && oldInfo.Kind != GlobPath { | ||||||||
| break | ||||||||
| } | ||||||||
| for newPath, newSlices := range paths { | ||||||||
| if oldPath == newPath { | ||||||||
|
|
||||||||
| prefixLen := strings.IndexAny(oldPath, "*?") | ||||||||
| if prefixLen == -1 { | ||||||||
| return fmt.Errorf("internal error: invalid path: generate or glob path does not contain ' ?' or '*': %q", oldPath) | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| } | ||||||||
| searchKey := oldPath[:prefixLen] | ||||||||
| // startIndex is the position of the prefix or the position where | ||||||||
| // the prefix would have been found. | ||||||||
| startIndex, _ := slices.BinarySearch(allPaths, searchKey) | ||||||||
| for i := startIndex; i < len(allPaths); i++ { | ||||||||
| newPath := allPaths[i] | ||||||||
| if !strings.HasPrefix(newPath, searchKey) { | ||||||||
| // Iterate until the prefix no longer matches, which means | ||||||||
| // all other paths will fail the comparison below. | ||||||||
| break | ||||||||
| } | ||||||||
| newSlices := paths[newPath] | ||||||||
| // We know prefixes match, we can skip that part of the string. | ||||||||
| if oldPath[len(searchKey)-1:] == newPath[len(searchKey)-1:] { | ||||||||
| // Identical paths have been filtered earlier. | ||||||||
| continue | ||||||||
| } | ||||||||
|
|
@@ -291,13 +325,16 @@ func (r *Release) validate() error { | |||||||
| continue | ||||||||
| } | ||||||||
| } | ||||||||
| if strdist.GlobPath(newPath, oldPath) { | ||||||||
| // We know prefixes match, we can skip that part of the string. | ||||||||
| if strdist.GlobPath(newPath[len(searchKey)-1:], oldPath[len(searchKey)-1:]) { | ||||||||
| if (old.Package > new.Package) || (old.Package == new.Package && old.Name > new.Name) || | ||||||||
| (old.Package == new.Package && old.Name == new.Name && oldPath > newPath) { | ||||||||
| old, new = new, old | ||||||||
| oldPath, newPath = newPath, oldPath | ||||||||
| } | ||||||||
| return fmt.Errorf("slices %s and %s conflict on %s and %s", old, new, oldPath, newPath) | ||||||||
| } else { | ||||||||
| break | ||||||||
|
Comment on lines
+336
to
+337
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I cannot confidently tell if this is changing anything. Is that to speed things up by skipping remaining slices in |
||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the most time-consuming of almost every command before the change. I think the comment should help understand the current behavior, not the old one that was improved.