This repository was archived by the owner on Oct 4, 2020. It is now read-only.
More efficient toUnfoldable without the Ord constraint #76
Conversation
This file contains hidden or 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
Keep a List of Maps as the unfold state, instead of a single Map. Unioning Maps was linear time (making the entire function quadratic?). Prepending to a List is constant time and doesn't require an `Ord k` instance.
hdgarrood
reviewed
Dec 7, 2016
| Two left k v right -> | ||
| Just $ Tuple (Tuple k v) (left : right : tl) | ||
| Three left k1 v1 mid k2 v2 right -> | ||
| Just $ Tuple (Tuple k1 v1) ((singleton k2 v2) : left : mid : right : tl) |
There was a problem hiding this comment.
Minor nit: I'd remove the brackets around singleton k2 v2.
|
Here's benchmark results for smaller maps, showing the quadratic behavior:
It gets really slow for big maps. The stack blows with maps sized around 10,000 because List's append is not stack-safe. The new version doesn't have this issue because it's not using |
|
Wow, that's pretty significant! Ok then, LGTM 👍 |
|
Awesome, thanks! |
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Keep a List of Maps as the unfold state instead of a single Map.
Appending Maps together was linear time (making the entire function quadratic?).
Prepending to a List is constant time and doesn't require an
Ord kinstance.