Skip to content

transport: Re-use slice buffer reader for a stream #8360

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented May 26, 2025

This introduces a new method to reset the BufferSlice being used by a mem.Reader. The enables using a single Reader for an HTTP/2 stream instead of allocating one object per gRPC message. This reduces allocs/op, but doesn't have a noticable impact on qps or latency.

Benchmarks

 go run benchmark/benchresult/main.go before after
streaming-networkMode_Local-bufConn_false-keepalive_false-benchTime_30s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_60-reqSi
ze_32384B-respSize_32384B-compressor_off-channelz_false-preloader_false-clientReadBufferSize_-1-clientWriteBufferSize_-1-serverReadBuff
erSize_-1-serverWriteBufferSize_-1-sleepBetweenRPCs_0s-connections_1-recvBufferPool_simple-sharedWriteBuffer_false
               Title       Before        After Percentage
            TotalOps      1089062      1096640     0.70%
             SendOps            0            0      NaN%
             RecvOps            0            0      NaN%
            Bytes/op    135783.55    135744.35    -0.03%
           Allocs/op        32.20        30.05    -6.21%
             ReqT/op 9404849015.47 9470290602.67     0.70%
            RespT/op 9404849015.47 9470290602.67     0.70%
            50th-Lat   1.646019ms    1.62842ms    -1.07%
            90th-Lat   2.280214ms   2.264523ms    -0.69%
            99th-Lat   2.692703ms    2.68211ms    -0.39%
             Avg-Lat   1.651539ms   1.640149ms    -0.69%
           GoVersion     go1.24.2     go1.24.2
         GrpcVersion   1.74.0-dev   1.74.0-dev

RELEASE NOTES:

  • transport: TBD

@arjan-bal arjan-bal added this to the 1.74 Release milestone May 26, 2025
@arjan-bal arjan-bal added Type: Performance Performance improvements (CPU, network, memory, etc) Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. labels May 26, 2025
Copy link

codecov bot commented May 26, 2025

Codecov Report

Attention: Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.25%. Comparing base (32e57de) to head (e18b029).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
internal/transport/controlbuf.go 95.83% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8360      +/-   ##
==========================================
- Coverage   82.35%   82.25%   -0.10%     
==========================================
  Files         419      419              
  Lines       42034    42052      +18     
==========================================
- Hits        34615    34588      -27     
- Misses       5968     6001      +33     
- Partials     1451     1463      +12     
Files with missing lines Coverage Δ
internal/transport/http2_client.go 92.73% <100.00%> (+0.58%) ⬆️
internal/transport/http2_server.go 90.89% <100.00%> (-0.20%) ⬇️
mem/buffer_slice.go 96.42% <100.00%> (+0.15%) ⬆️
internal/transport/controlbuf.go 89.88% <95.83%> (-0.81%) ⬇️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@arjan-bal arjan-bal force-pushed the re-use-buffer-reader branch from ef4e1a9 to 6026288 Compare May 26, 2025 14:20
@arjan-bal arjan-bal requested a review from dfawley May 27, 2025 17:47
Comment on lines 157 to 161
r.Close()
s.Ref()
r.data = s
r.len = s.Len()
r.bufferIdx = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could optimize very slightly by not calling r.Close() and just calling r.data.Free() instead, since you are initializing every field here.

But...that makes me wonder: why does Close bother clearing data and len in the first place? I am thinking that's done intentionally "in case a read happens after Close". If so, then are we sure it is okay to re-use them?

cc @PapaCharlie in case he remembers the motivation for this and can comment on the feasibility about reusing the readers here.

Copy link
Contributor Author

@arjan-bal arjan-bal May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to avoid redundant work in Reset. The godoc for Close mentions that it's okay for readers to call Read after close.

Subsequent calls to Read will return (0, io.EOF).

If Read was not allowed after Close, we could have pooled the Reader objects, putting object back in the pool at the end of Close. With the Reset method, the caller gets to decide when to call Reset. If they plan to Read from the old buffer slice later, they should not call Reset.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. We might have some places that are reading after Close, but presumably the control buf isn't one of them.

@dfawley dfawley assigned arjan-bal and unassigned dfawley May 27, 2025
@arjan-bal arjan-bal assigned dfawley and unassigned arjan-bal May 28, 2025
Comment on lines 157 to 161
r.Close()
s.Ref()
r.data = s
r.len = s.Len()
r.bufferIdx = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. We might have some places that are reading after Close, but presumably the control buf isn't one of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. Type: Performance Performance improvements (CPU, network, memory, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants