alts: Release read buffer when blocked on socket read#8964
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8964 +/- ##
==========================================
+ Coverage 83.01% 83.09% +0.08%
==========================================
Files 411 413 +2
Lines 32960 33051 +91
==========================================
+ Hits 27361 27464 +103
+ Misses 4196 4191 -5
+ Partials 1403 1396 -7
🚀 New features to boost your workflow:
|
a937698 to
bc023c9
Compare
|
@arjan-bal : Can you please merge changes from |
easwars
left a comment
There was a problem hiding this comment.
I haven't looked much at the alts changes. Should we get someone from the security team to review that?
Adding @gtcooke94 for reviewing the ALTS changes. |
b169d72 to
f226417
Compare
easwars
left a comment
There was a problem hiding this comment.
LGTM for the non-alts files
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a ReadyReader interface and a SimpleBufferPool to optimize memory usage during ALTS record reads by avoiding long-term memory pinning when connections are idle. It includes platform-specific implementations for non-blocking reads on Unix systems and updates the ALTS connection logic to utilize these pooled buffers. The review feedback correctly identifies several compilation errors in the new test file ready_reader_test.go where values received from testutils.Channel (which return interface{}) are compared directly to integers without necessary type assertions.
a14f26a to
b6b7d6c
Compare
ef4bb79 to
9636c85
Compare
## Problem The `ReadyReader` interface was added to support non-blocking reads using the `syscall` package in #8964. While working on [follow-up changes](https://github.com/grpc/grpc-go/compare/master...arjan-bal:h2-read-buffer-pool?expand=1) to support pooling read buffers in the http2 layer, I noticed test failures where servers were closing the TCP connection, but the client channel remained in the READY state. ## Root Cause The [unix.Read](https://pkg.go.dev/golang.org/x/sys/unix#Read) function doesn't consider the graceful closure of a connection to be an error and instead returns `(0, nil)`. Since consumers of the `io.Reader` interface expect an `io.EOF` error in such cases, the translation from `(0, nil)` to `(0, io.EOF)` needs to be handled by the reader implementation. RELEASE NOTES: N/A
## Problem The HTTP/2 framer in gRPC uses a `bufio.Reader` with a 32KB buffer by default. When there are a large number of transports, these buffers consume significant memory, even when the transport is idle. ## Solution #8964 added a `ReadyReader` interface that allows non-memory-pinning reads. This PR replaces the standard `bufio.Reader` with a custom `io.Reader` implementation that uses pooled buffers and releases the buffer once all data is consumed. To defer the re-allocation of the read buffer, the reader calls `ReadOnReady` on the underlying `io.Reader`. For this to work, the underlying `io.Reader` must implement either the `ReadyReader` interface or `syscall.RawConn`. If neither condition is met, the framer gracefully falls back to using the regular `bufio.Reader`. Additional Changes: * The ALTS connection has been refactored to implement the `ReadyReader` interface. * The write buffer pools used by the framer are updated to use the `mem.BufferPool` interface, allowing the pools to be shared across both read and write operations. * Use `syscall.Read` instead of `unix.Read` to avoid triggering the race detector, see comment for details. * Add environment variable protection for the changes to allow fast rollback. ## Benchmarks In a [real-world benchmark](https://github.com/arjan-bal/custom-go-client-benchmark/tree/retry-dp), where a GCS directpath client downloads a file in a loop, the average "in use" memory falls from 28.3MB to 21.3MB (-24%). Local Benchmarks show no significant difference ``` ❯ go run benchmark/benchresult/main.go streaming-before streaming-after streaming-networkMode_Local-bufConn_false-keepalive_false-benchTime_2m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCa lls_120-reqSize_1024B-respSize_1024B-compressor_off-channelz_false-preloader_false-clientReadBufferSize_-1-clientWriteBuffer Size_-1-serverReadBufferSize_-1-serverWriteBufferSize_-1-sleepBetweenRPCs_0s-connections_1-recvBufferPool_simple-sharedWrite Buffer_true Title Before After Percentage TotalOps 29981273 29966908 -0.05% SendOps 0 0 NaN% RecvOps 0 0 NaN% Bytes/op 4971.06 4971.41 0.00% Allocs/op 19.79 19.79 0.00% ReqT/op 2046721570.13 2045740919.47 -0.05% RespT/op 2046721570.13 2045740919.47 -0.05% 50th-Lat 461.523µs 460.906µs -0.13% 90th-Lat 654.435µs 655.327µs 0.14% 99th-Lat 1.225856ms 1.240984ms 1.23% Avg-Lat 478.845µs 479.553µs 0.15% GoVersion go1.25.0 go1.25.0 GrpcVersion 1.81.0-dev 1.81.0-dev ``` 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.
Problem
Normally, since the Go
net.Conninterface provides the abstraction of a blocking read to hide the complexity of non-blocking I/O (epoll, kqueue), users need to pass a read buffer to thenet.Conn.Readcall. Even when the TCP socket doesn't have data, the application needs to hold onto the read buffer. This results in the ALTS conn and the gRPC HTTP/2 stack holding on to 32KB read buffers each, even for non-readable transports.Solution
On Unix platforms, there is a RawConn interface that exposes a non-blocking mechanism. The idea is that the Go runtime will call a user-registered callback when the socket is readable. gRPC can use this callback to allocate a buffer from the pool and return it once it has passed the plaintext to the HTTP/2 layer. The same RawConn interface is also available in other OSs, but with slightly different method signatures for the blocking
Readmethod. In the future, we can add the same optimization for them and have CI runners to catch regressions.The main abstraction that allows these non-memory-pinning reads is the following interface:
In this PR, an implementation is provided that wraps a
RawConn. This allows the ALTS conn to perform efficient reads.In a future PR, the following changes will enable getting rid of the bufio.Reader in the HTTP/2 layer:
ReadyReaderwill be implemented by the ALTS conn.ReadOnReady()instead ofRead()on the underlying conn, if supported, to delay the re-allocation of the buffer.Benchmarks
The following micro-benchmarks show no regression in QPS (LargeMessage test) and a significant reduction in memory usage while performing reads (ReadMemoryUsage). There is an increase in 2 allocs in conn construction due to the use of pointer fields for the
ReadyReaderand read buffer handle, but these happen only when creating a subchannel, not per-RPC. There is an increase in sec/op for theWriteMemoryUsagetest, but these tests are not meant to measure conn construction time, only the memory effeciency.In a real-world benchmark, where a GCS directpath client downloads a file in a loop, the average "in use" memory falls from ~35MB to ~20MB.
RELEASE NOTES: