-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
perf(storage): gRPC zerocopy codec #10888
Conversation
3392f73
to
5a1e3a1
Compare
Replace the current custom codec for ReadObjectResponse with a CodecV2 that can handle data split across multiple buffers. Unit tests pass but need to finish up some stuff to get the end-to-end working.
5a1e3a1
to
099a5e9
Compare
storage/grpc_client.go
Outdated
// Consume a bytes field from the input. Returns offsets for the data in the buffer slices | ||
// and an error. | ||
func (d *readResponseDecoder) consumeBytes() (*bufferSliceOffsets, error) { | ||
b := (*d.databufs)[d.currBuf].ReadOnlyData() |
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.
nit, this function only seems to require (*d.databufs)[d.currBuf].Len()
.
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.
Couple initial comments, looks like there is some commented out code as well.
0723415
to
20e4a84
Compare
storage/grpc_client.go
Outdated
// Consume a varint that represents the length of a bytes field. Return the length of | ||
// the data, and advance the offsets by the length of the varint. | ||
func (d *readResponseDecoder) consumeVarint() (uint64, error) { | ||
b := (*d.databufs)[d.currBuf].ReadOnlyData() |
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 wonder if it's be worth stashing the current buffer's []byte in readResponseDecoder, to avoid the need for repeated calls to ReadOnlyData.
Or perhaps even stash a [][]byte and materialize all the buffers once at the start of decoding. You could avoid an allocation in the common case by keeping a [N][]byte in the decoder:
type readResponseDecoder struct {
bufs [][]byte
staticBufs [4][]byte
}
func (d *readResponseDecoder) init() {
d.bufs = d.staticBufs[:0]
for _, b := range d.databufs {
d.bufs = append(d.bufs, b.ReadOnlyData())
}
}
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 don't fully understand this, why would staticBufs be of len 4?
I think I'll probably just leave this for now if that's okay, I don't see overhead from these calls.
94062c4
to
66bc174
Compare
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.
Looks good to me!
currOff := d.currOff | ||
var buf []byte | ||
for remaining > 0 { | ||
b := d.databufs[currBuf].ReadOnlyData() |
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.
Could simplify the code a tiny bit by making b start at the current offset:
b := b.databufs[currBuf].ReadOnlyData()[currOff:]
if len(b) < remaining {
buf = append(buf, b...)
remaining -= len(b)
currBuf++
currOff = 0
} else {
buf = append(buf, b[:remaining]...)
remaining = 0
}
d.currOff += remaining | ||
remaining = 0 | ||
} | ||
} |
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.
Might be a bit simpler as:
d.currOff += n
for d.currBuf < len(d.databufs) && d.currOff >= d.databufs[d.currBuf].Len() {
d.currOff -= d.databufs[d.currBuf].Len()
d.currBuf++
}
Replace the current custom codec for ReadObjectResponse with a CodecV2 that can handle data split across multiple buffers.
Unit tests pass but need to finish up some stuff to get the end-to-end working.