-
Notifications
You must be signed in to change notification settings - Fork 139
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
zipWith'
to packZipWith
as per #208
#295
Conversation
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.
Cool! Please do the same for Data.ByteString.Char8
, Data.ByteString.Lazy
and Data.ByteString.Lazy.Char8
.
I'm not sure how comfortable I am replicating |
I added packZipWith to Data.ByteString.Char8 since the function from Data.ByteString only requires Storable a. |
d07ee50
to
e0946d1
Compare
You can impement bytestring/Data/ByteString/Lazy/Internal.hs Lines 251 to 265 in d701a2a
|
On it |
Great! Please add tests for new functions. |
I've only added test cases where there were ones for the packZipWith previously. If there are any more needed just ask |
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.
LGTM!
EQ -> packZipWith f as bs | ||
GT -> packZipWith f (Chunk (S.drop bl a) as) bs | ||
{-# INLINE packZipWith #-} | ||
|
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.
How about adding a RULE similar to the one for the strict 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.
Given that in future we intend to decomission the rewrite rule, we can probably remove the sentence advertising it already.
- Bodigrim
Don't know how I feel about adding a rule only for it to be decommisioned later, still added the rule to Data.ByteString,Lazy though
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.
Instead of copying a rule from the strict version, I would rather add
{-# RULES "Specialise zipWith" pack (zipWith f p q) = packZipWith f p q #-}
(both for strict and lazy bytestrings)
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.
After reading #208 again I agree. I changed the rewrite rule
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.
Data/ByteString.hs:1649:1: warning: [-Winline-rule-shadowing]
Rule "ByteString specialise zipWith" may never fire
because ‘pack’ might inline first
Probable fix: add an INLINE[n] or NOINLINE[n] pragma for ‘pack’
|
1649 | "ByteString specialise zipWith" forall (f :: Word8 -> Word8 -> Word8) p q .
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...
GHC is complaining. I'm not that read up on the inline/noinline pragmas. What needs to be changed?
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 added {-# INLINE [1] pack #-}
to Data.ByteString and Data.ByteString.Lazy since they only resolve to packBytes and I'm sure it would get inlined anyways.
I added {-# INLINE [1] zipWith #-}
to Data.ByteString.Lazy but I'm not so sure about the performance characteristics of that. Shouldn't be too big of an issue since it just resolves to go a as b bs
.
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.
Thanks for experimenting. I don't mind {-# INLINE [1] zipWith #-}
, but not quite sure about {-# INLINE [1] pack #-}
. It seems innocent, but deserves a thorough investigation of the impact, because pack
is used pretty much everywhere.
To stop going down the rabbit hole, let's reset back to d93cdd4 and be with it for now. Rules issue can be resolved in a separate PR later. Sorry that it became so laborious.
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.
Apologies for the confusion – I had missed the discussion in #208.
523754a
to
d93cdd4
Compare
@elikoga Thanks, well done! |
Renames
zipWith'
topackZipWith
and exports itRecovers test cases for
packZipWith
from commentsAll test cases pass