-
Notifications
You must be signed in to change notification settings - Fork 180
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
Improve compare for IntSet and IntMap #1086
Merged
Merged
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
2f95084
Benchmarks for compare
meooow25 7d90d01
Improve compare for IntSet and IntMap
meooow25 13a1bd3
Adopt splitSign
meooow25 5b6ee3a
Make sure leftmostTipSure returns unboxed values
meooow25 6633dd7
Update changelog
meooow25 a85b6fb
Rename Order constructors
meooow25 030fb5e
Comment on orderTip internals
meooow25 c9eb5b7
Work around pre-9.0 Haddock misbehaving
meooow25 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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -212,6 +212,7 @@ import Data.IntSet.Internal.IntTreeCommons | |
, TreeTreeBranch(..) | ||
, treeTreeBranch | ||
, i2w | ||
, Order(..) | ||
) | ||
|
||
#if __GLASGOW_HASKELL__ | ||
|
@@ -1486,8 +1487,94 @@ equal _ _ = False | |
--------------------------------------------------------------------} | ||
|
||
instance Ord IntSet where | ||
compare s1 s2 = compare (toAscList s1) (toAscList s2) | ||
-- tentative implementation. See if more efficient exists. | ||
compare = compareIntSets | ||
|
||
compareIntSets :: IntSet -> IntSet -> Ordering | ||
compareIntSets s1 s2 = case (splitSign s1, splitSign s2) of | ||
((l1, r1), (l2, r2)) -> case go l1 l2 of | ||
A_LT_B -> LT | ||
A_Prefix_B -> if null r1 then LT else GT | ||
A_EQ_B -> case go r1 r2 of | ||
A_LT_B -> LT | ||
A_Prefix_B -> LT | ||
A_EQ_B -> EQ | ||
B_Prefix_A -> GT | ||
A_GT_B -> GT | ||
B_Prefix_A -> if null r2 then GT else LT | ||
A_GT_B -> GT | ||
where | ||
go t1@(Bin p1 l1 r1) t2@(Bin p2 l2 r2) = case treeTreeBranch p1 p2 of | ||
ABL -> case go l1 t2 of | ||
A_Prefix_B -> A_GT_B | ||
A_EQ_B -> B_Prefix_A | ||
o -> o | ||
ABR -> A_LT_B | ||
BAL -> case go t1 l2 of | ||
A_EQ_B -> A_Prefix_B | ||
B_Prefix_A -> A_LT_B | ||
o -> o | ||
BAR -> A_GT_B | ||
EQL -> case go l1 l2 of | ||
A_Prefix_B -> A_GT_B | ||
A_EQ_B -> go r1 r2 | ||
B_Prefix_A -> A_LT_B | ||
o -> o | ||
NOM -> if unPrefix p1 < unPrefix p2 then A_LT_B else A_GT_B | ||
go (Bin _ l1 _) (Tip k2 bm2) = case leftmostTipSure l1 of | ||
Tip' k1 bm1 -> case orderTips k1 bm1 k2 bm2 of | ||
A_Prefix_B -> A_GT_B | ||
A_EQ_B -> B_Prefix_A | ||
o -> o | ||
go (Tip k1 bm1) (Bin _ l2 _) = case leftmostTipSure l2 of | ||
Tip' k2 bm2 -> case orderTips k1 bm1 k2 bm2 of | ||
A_EQ_B -> A_Prefix_B | ||
B_Prefix_A -> A_LT_B | ||
o -> o | ||
go (Tip k1 bm1) (Tip k2 bm2) = orderTips k1 bm1 k2 bm2 | ||
go Nil Nil = A_EQ_B | ||
go Nil _ = A_Prefix_B | ||
go _ Nil = B_Prefix_A | ||
|
||
-- This type allows GHC to return unboxed ints from leftmostTipSure, as | ||
-- $wleftmostTipSure :: IntSet -> (# Int#, Word# #) | ||
-- On a modern enough GHC (>=9.4) this is unnecessary, we could use StrictPair | ||
-- instead and get the same Core. | ||
data Tip' = Tip' {-# UNPACK #-} !Int {-# UNPACK #-} !BitMap | ||
|
||
leftmostTipSure :: IntSet -> Tip' | ||
leftmostTipSure (Bin _ l _) = leftmostTipSure l | ||
leftmostTipSure (Tip k bm) = Tip' k bm | ||
leftmostTipSure Nil = error "leftmostTipSure: Nil" | ||
|
||
orderTips :: Int -> BitMap -> Int -> BitMap -> Order | ||
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. Please add a haddock string. |
||
orderTips k1 bm1 k2 bm2 = case compare k1 k2 of | ||
LT -> A_LT_B | ||
EQ | bm1 == bm2 -> A_EQ_B | ||
| otherwise -> | ||
-- To lexicographically compare the elements of two BitMaps, | ||
-- * Find the lowest bit where they differ. | ||
-- * For the BitMap with this bit 0, check if all higher bits are also | ||
-- 0. If yes it is a prefix, otherwise it is greater. | ||
let diff = bm1 `xor` bm2 | ||
lowestDiff = diff .&. negate diff | ||
highMask = negate lowestDiff | ||
in if bm1 .&. lowestDiff == 0 | ||
then (if bm1 .&. highMask == 0 then A_Prefix_B else A_GT_B) | ||
else (if bm2 .&. highMask == 0 then B_Prefix_A else A_LT_B) | ||
GT -> A_GT_B | ||
{-# INLINE orderTips #-} | ||
meooow25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
-- Split into negative and non-negative | ||
splitSign :: IntSet -> (IntSet, IntSet) | ||
splitSign t@(Bin p l r) | ||
| signBranch p = (r, l) | ||
| unPrefix p < 0 = (t, Nil) | ||
| otherwise = (Nil, t) | ||
splitSign t@(Tip k _) | ||
| k < 0 = (t, Nil) | ||
| otherwise = (Nil, t) | ||
splitSign Nil = (Nil, Nil) | ||
{-# INLINE splitSign #-} | ||
|
||
{-------------------------------------------------------------------- | ||
Show | ||
|
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.
Please add a Haddock string.
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.
Why? These functions are not exposed.
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.
My general philosophy is that every top level function and every non-trivial function should be fully documented. For
leftmostTipSure
, I recognize that the name really gives it away, but I'm stubborn.orderTips
must surely have a documentable purpose, with some expectations about what its arguments will mean and some description of what its result means.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.
Sorry, I disagree. There is nothing to be gained by adding noise to internal functions with self-explanatory name+type.
If you insist on this please provide the doc strings you would like them to have.
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.
@treeowl do you still want to add doc strings?
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'll try to write one for
compareTips
by tomorrow night. But otherwise I guess you can merge and I'll open an issue to remember.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'll go ahead and merge it then.