-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add buildMessageDelimited: size-delimited streams of Messages #102
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
Note: This currently typechecks but is not tested, testing locally next. |
Note that #61 has some prior discussion of this feature. |
Tested to work via https://github.com/LukeHoersten/prometheus/pull/7 |
I signed it |
4a22085
to
e643b17
Compare
mempty = SizedBuilder 0 mempty | ||
mappend = (<>) | ||
|
||
putVarInt :: Word64 -> SizedBuilder |
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.
Copy this from Data.ProtoLens.Encoding.Bytes
?
| otherwise = Builder.word8 (fromIntegral $ n .&. 127 .|. 128) | ||
<> putVarInt (n `shiftR` 7) | ||
-- | A wrapper around a 'Builder' that keeps track of its size. | ||
data SizedBuilder = SizedBuilder |
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.
Would it be simpler to just call toLazyByteString
(or toStrict . toLazyByteString
) on a builder and then get its size?
It's hard for me to reason about the performance tradeoffs of this change. We have some benchmarks in this repo in proto-lens-benchmarks
; I'd be reluctant to refactor how the building code works without seeing a noticeable improvement in those benchmarks (or a new one). I could anticipate slowdowns caused by a space leak in the _size
parameter (it's hard for me to reason about off the top of my head), or due to the redundant additions/bookkeeping, or due to confusing GHC's optimizer.
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 wary of the lazy _size
as well. If we want to maximize encoding speed, I would expect that the approach used by http://hackage.haskell.org/package/cborg with an intermediate custom tailored token stream and a hot encoder loop would perform best. See: https://youtu.be/1WrSPSYhE6o?t=28m23s
putVarInt' n | ||
| n < 128 = Builder.word8 (fromIntegral n) | ||
| otherwise = Builder.word8 (fromIntegral $ n .&. 127 .|. 128) | ||
<> putVarInt' (n `shiftR` 7) |
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.
@judah @shlevy note that you could use http://hackage.haskell.org/package/bytestring-0.10.8.2/docs/Data-ByteString-Builder-Prim.html to avoid the recursion completely and perform the check whether the buffer is large enough in a single step. See the encodeCharUtf8HtmlEscaped
example for inspiration.
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.
Recalling the times when I was working on the bytestring builder, I'd expect about an order-of-magnitude speedup for encoding varInt
s with this change.
e643b17
to
2aa1afb
Compare
@judah removed premature optimization |
Added testReluGrad, testReluGradGrad and testFillGrad
No description provided.