-
Notifications
You must be signed in to change notification settings - Fork 92
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
chunkmatches: reuse last calculated column when filling #711
Merged
Merged
Changes from 2 commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
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.
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.
Just to check: are we always sure we can assume our candidates are increasing in offset? I can't remember if this is always true.
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.
Oh, reading the implementation, I guess we just fall back to the less performant version.
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.
I believe this invariant is true, because in
gatherMatches
we always make sure to sort bybyteOffset
.Maybe we could update the comments to make it clear this invariant is assumed, and treat the unsorted case as an error rather than being expected? That way if we ever introduce a bug here, we don't silently fall back to an O(n^2) algorithm... much harder to track down than a clear error in testing.
General thought: if invariants are too tricky to reason about, sometimes I just explicitly add a (re)sort! I believe Go's default sort is very fast when the input is already sorted. This bounds the worst case nicely.
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.
I also checked the invariants. We also have the invariant that things don't overlap, which is also important since we lookup the end column.
The sorted invariant is actually quite important for other bits of code like chunkCandidates. So what I did was add a sorted check which loudly complains and then sorts if the invariant is broken.
Initially I pretended I was a haskell programmer and added a special type which guaranteed this, but TBH it felt quite overengineered. Happy to try it out if there is interest, but for now gonna merge with extra perf invariant documentation and sort check.