-
Notifications
You must be signed in to change notification settings - Fork 97
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
#254, fix space leak on collisions #258
Merged
treeowl
merged 8 commits into
haskell-unordered-containers:master
from
ndmitchell:patch-1
Jun 2, 2020
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
647f6af
#254, avoid space leak with collisions
ndmitchell d1d0151
#254, add a regression test
ndmitchell 54bcfdd
Improve the documentation on the regression test
ndmitchell dea714a
More inline comments on the space leak regression test
ndmitchell 208e450
Add random package as a dependency
ndmitchell 6ebe947
Update Regressions.hs
ndmitchell b6ed146
Refactor the updateOrSnocWithKey otherwise case
ndmitchell c8ff7b4
Alphabetise the package dependencies
ndmitchell 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
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
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.
Maybe add a reference to David's explanation of the unboxed-tuple trick here?
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.
We use the unboxed tuple trick in a bunch of places in HashMap, so it seems "expected knowledge" for working with this package.
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.
Perhaps we should raise a general doc issue for this trick then. It's uncommon knowledge I think 😄
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 think a blog post explaining the trick would be great. I wouldn't document it in the context of unordered-containers, but try and make it a general technique, and just link to it from this project.
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.
Yeah, a blog post would be great. I have opened #262 as a reminder.
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 would call this
updateOrSnocWithKey#
, and defineupdateOrSnocWithKey
to use it in the boring way. As it is, this function can only be used for the lazy operations. Should we extend it to let it work for strict ones too? The most semantically pleasing version would look like this:This takes a function that we pass either
(# (# #) | #)
(meaning the key was not present) or(# | v #)
(the value tied to the key in the map) and produces an unboxed unary tuple for the new value.Unfortunately, we support GHC versions that don't have unboxed sums. We can work around that in one of two ways:
Another issue is evaluating the whole
v -> v
vs.v -> v -> v
sort of approach, which has to do with what closures we might have to create in different contexts. This will come down to poring over Core and doing some benchmarking. This is actually true whether we try to unify strict and lazy or not. Personally, I'd love to be able to unify as much as we can, if we can get enough of the right things to inline the way we want to avoid hurting performance.Final note: if we're careful enough about inlining, we might well be able to just use boxed everything and still not pay for it. That means making sure that the underlying implementation inlines into the
Strict
orLazy
version as soon as the wrapped-up function is passed in, while thatStrict
orLazy
version itself remains (at least)INLINABLE
.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.
Is it useful to add both
updateOrSnocWith#
andupdateOrSnocWith
? For user code, where we expose it, I agree that having a "friendly" wrapper is valuable. But given we haveupdateOrSnocWith
andupdateOrSnocWithKey
that seems like a lot of intermediate helper functions that don't get used a whole lot.My inclination would be to keep this patch (which fixes a space leak) a small self-contained space leak fix, and then follow up with the larger unboxed sums and looking at the Core after. It feels like a subsequent refactor, and one that I'm not well placed to make.