readyreader: add buffered reader#9055
Conversation
|
/gemini review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9055 +/- ##
==========================================
+ Coverage 82.91% 83.15% +0.23%
==========================================
Files 413 413
Lines 33404 33430 +26
==========================================
+ Hits 27698 27799 +101
+ Misses 4230 4218 -12
+ Partials 1476 1413 -63
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request generalizes the readyreader package to accept io.Reader instead of net.Conn and introduces bufReadyReader to provide a buffered io.Reader interface over a ReadyReader. The changes include updated logic for non-blocking reader detection and comprehensive new tests. Review feedback identifies a potential buffer leak in the Read implementation when zero bytes are returned and highlights several documentation inconsistencies and truncated comments that need correction.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
easwars
left a comment
There was a problem hiding this comment.
Changes look mostly good. I still haven't looked at the tests yet.
| if b.err != nil { | ||
| return 0, b.readErr() | ||
| } | ||
| if len(p) >= b.bufSize { |
There was a problem hiding this comment.
Would it nicer to structure this as follows:
if b.r == b.w {
...
var pool mem.BufferPool
if len(p) >= b.bufSize {
b.constPool.buffer = p
pool = &b.constPool
} else {
pool = b.pool
}
// Rest is common for both paths nowThere was a problem hiding this comment.
If we're reading directly into the caller's buffer, we need to return early to avoid modifying the read/write pointers. Combining these cases would require adding another check further down to handle the early return.
|
@arjan-bal Did you forget to send your replies to my comments? |
Original PRs: #9055, #9032 RELEASE NOTES: * transport: Pool HTTP/2 framer read buffers to reduce idle memory consumption. Currently limited to Linux for ALTS and non-encrypted transports (TCP, Unix). To disable, set `GRPC_GO_EXPERIMENTAL_HTTP_FRAMER_READ_BUFFER_POOLING=false` and report any issues. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This PR introduces a buffered
io.Readerthat automatically releases its read buffer when empty. To optimize memory usage, the reader defers buffer reallocation until data is available in the underlyingreadyreader.Reader(achieved by callingReadOnReady).In a follow-up PR (#9032), the HTTP/2 framer will be updated to utilize this new buffered reader whenever the underlying reader implements the
readyreader.Readerinterface.The implementation and associated tests are based on the standard library's.
RELEASE NOTES: N/A