Refactor eachregion to be O(n log n) not O(n^2) #73
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
|
Looks like I've got an off-by-one error to fix, but other than that this seems solid. |
tecosaur
force-pushed
the
presort-in-eachregion
branch
2 times, most recently
from
1044edf to
fb40c04
Compare
Since we removed the ordering restriction on annotations to improve the semantics of annotation modification, each `annotations(str)` call became `O(n)` which is fine for a once off, but use it in a loop as `eachregion` does and now it's `O(n m)`. That's pretty underwhelming. We can improve this to `O(n log n)` by pre-sorting the list of annotations, and working with it instead. A bit more complexity is needed to do this while preserving the semantics, but it can be worth it for long strings. With a 100,000 char string with 20,000 annotations, print time goes from ~0.4s to ~0.01s on my machine.
tecosaur
force-pushed
the
presort-in-eachregion
branch
from
fb40c04 to
c417262
Compare
While this is implicitly tested later on, I think it's nice to test for it explicitly. If nothing else, should any potentially buggy modifications be made in the future it will make it easier to pin down the root misbehaviour.
|
This has ended up being a ~40x performance improvement to |
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.
Since we removed the ordering restriction on annotations to improve the semantics of annotation modification, each
annotations(str)call becameO(n)which is fine for a once off, but use it in a loop aseachregiondoes and now it'sO(n m). That's pretty underwhelming.We can improve this to
O(n log n)by pre-sorting the list of annotations, and working with it instead. A bit more complexity is needed to do this while preserving the semantics, but it can be worth it for long strings. With a 100,000 char string with 20,000 annotations, print time goes from ~0.4s to 0.015s on my machine.This improvement has been prompted by #72.