-
Notifications
You must be signed in to change notification settings - Fork 453
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
[dbnode] Decode ReadBits improvements #2197
Changes from all commits
931e91d
7579ea5
ee74aef
b918b98
962ea06
9bad391
23a97ca
ecb415f
4631252
324217f
d5a8ca0
dec5fbd
a06d51c
cbcff35
b27f2b7
57d0a99
8b181f4
f3856e5
f107e89
b092ae2
653a6b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ import ( | |
const ( | ||
defaultDefaultTimeUnit = xtime.Second | ||
defaultByteFieldDictLRUSize = 4 | ||
defaultIStreamReaderSizeM3TSZ = 16 | ||
defaultIStreamReaderSizeM3TSZ = 8 * 2 | ||
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. Also FYI I was playing around with this setting since it drives how large a buffer we keep in this stream. Seems like it does have some effects on the flamegraphs where the larger buffer avoids time going to the buffer 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. Interesting, yeah this might be worth playing around with - potentially in a way that only effects query instead of definitely dbnode as well. |
||
defaultIStreamReaderSizeProto = 128 | ||
) | ||
|
||
|
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 are we removing the error? Shouldn't ReadBit still fail if the stream has failed?
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.
+1
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.
This error is set from the result of
readByteFromStream
, but in all of the cases we call this, the parent call also returns that error (e.g.ReadBit
). So I see no reason to keep the error stored as state on the stream object itself if we already would have returned it. Is there a reason to keep it, though, that I'm missing? The value of removing it is that we can then remove these if-conditions that are in multiple methods to check if the error is set.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 there a noticeable difference on flame graphs? Branch prediction should help reduce the cost of these when there's no error
Mostly concerned about losing some future proofing if the actual impact turns out to be very small
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 it is small but noticeable on the flamegraph. Most of
ReadBits
is attributable toreadByte
, and that increases when we remove this err checks (meaning less CPU work in theReadBits
func, as you can see on the far right of theReadBits
in the graphs).BEFORE (
readByte
is 10.62%)AFTER (
readByte
is 13.31%)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 I'd still lean on the side of caution here; otherwise there's a bit of a weird reliance that
is.r.Byte()
will error in the expected fashion otherwise, and if the underlying reader changes you may get weird behaviourThere 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.
Ok sounds good to me. I'll update to only include the casting changes.