fix(storage): revert optimize gRPC writer with zero-copy #14630
fix(storage): revert optimize gRPC writer with zero-copy #14630cpriti-os wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies the gRPCWriter by removing the zero-copy write path and transitioning from lazy to immediate buffer allocation. Feedback from the reviewer highlights that immediate allocation increases memory overhead and that relying on cap(w.buf) instead of a dedicated chunkSize field could result in a division-by-zero panic if the chunk size is zero. It is recommended to re-introduce lazy allocation and the chunkSize field to ensure robustness and efficiency.
| if len(w.buf)+len(v.p) <= cap(w.buf) { | ||
| // We have not started sending yet, and we can stage all data without | ||
| // starting a send. Compare against cap(w.buf) instead of | ||
| // w.writeQuantum: that way we can perform a oneshot upload for objects | ||
| // which fit in one chunk, even though we will cut the request into | ||
| // w.writeQuantum units when we do start sending. | ||
| origLen := len(w.buf) | ||
| w.buf = w.buf[:origLen+len(v.p)] | ||
| copy(w.buf[origLen:], v.p) | ||
| close(v.done) | ||
| } else { | ||
| // Too large. Handle it in writeLoop. | ||
| w.currentCommand = cmd | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This logic assumes cap(w.buf) is non-zero. If chunkSize is 0 (which is a valid setting in the storage package), this could lead to a division-by-zero panic in the handle method. Re-introducing lazy allocation with a chunkSize field and ensuring a minimum value would be safer.
| if len(w.buf)+len(v.p) <= cap(w.buf) { | |
| // We have not started sending yet, and we can stage all data without | |
| // starting a send. Compare against cap(w.buf) instead of | |
| // w.writeQuantum: that way we can perform a oneshot upload for objects | |
| // which fit in one chunk, even though we will cut the request into | |
| // w.writeQuantum units when we do start sending. | |
| origLen := len(w.buf) | |
| w.buf = w.buf[:origLen+len(v.p)] | |
| copy(w.buf[origLen:], v.p) | |
| close(v.done) | |
| } else { | |
| // Too large. Handle it in writeLoop. | |
| w.currentCommand = cmd | |
| return nil | |
| } | |
| if w.buf == nil { | |
| w.buf = make([]byte, 0, w.chunkSize) | |
| } | |
| if len(w.buf)+len(v.p) <= w.chunkSize { | |
| // We have not started sending yet, and we can stage all data without | |
| // starting a send. Compare against w.chunkSize instead of | |
| // w.writeQuantum: that way we can perform a oneshot upload for objects | |
| // which fit in one chunk, even though we will cut the request into | |
| // w.writeQuantum units when we do start sending. | |
| origLen := len(w.buf) | |
| w.buf = w.buf[:origLen+len(v.p)] | |
| copy(w.buf[origLen:], v.p) | |
| close(v.done) | |
| } else { | |
| // Too large. Handle it in writeLoop. | |
| w.currentCommand = cmd | |
| return nil | |
| } |
|
|
||
| buf: nil, // Allocated lazily on first buffered write. | ||
| chunkSize: chunkSize, | ||
| buf: make([]byte, 0, chunkSize), |
There was a problem hiding this comment.
Pre-allocating the buffer here increases memory overhead for every writer instance. Consider keeping the lazy allocation pattern (initializing buf to nil and storing chunkSize in the struct) to improve memory efficiency, especially when many writers are opened or for small uploads.
| buf: make([]byte, 0, chunkSize), | |
| buf: nil, | |
| chunkSize: chunkSize, |
|
|
||
| buf []byte | ||
| chunkSize int | ||
| buf []byte |
There was a problem hiding this comment.
This reverts commit df64147.