- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.6k
transport: Avoid buffer copies when reading Data frames #8657
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
Conversation
| Codecov Report❌ Patch coverage is  Additional details and impacted files@@            Coverage Diff             @@
##           master    #8657      +/-   ##
==========================================
- Coverage   83.29%   83.29%   -0.01%     
==========================================
  Files         415      415              
  Lines       32134    32196      +62     
==========================================
+ Hits        26767    26817      +50     
- Misses       4001     4005       +4     
- Partials     1366     1374       +8     
 🚀 New features to boost your workflow:
 | 
b5f777e    to
    ae8c8df      
    Compare
  
    efe661b    to
    778af0a      
    Compare
  
    778af0a    to
    45e856f      
    Compare
  
    45e856f    to
    5ec4a4e      
    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.
I realised that the changes in the std lib framer are not imported in g3 os this PR can't be merged in gRPC yet. I've added a Blocked label to indicate this.
| 
 When is it expected to be imported? | 
| 
 I think importing the code is self-service. I'm working on it: cl/824893868 | 
| @dfawley: Moving to your plate in case you want to take a look. Thanks. | 
| t.controlBuf.put(&outgoingWindowUpdate{s.id, w}) | ||
| } | ||
| } | ||
| // TODO(bradfitz, zhaoq): A copy is required here because there is no | 
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.
🎉
| if memPool == nil { | ||
| // Note that this is only supposed to be nil in tests. Otherwise, stream | ||
| // is always initialized with a BufferPool. | ||
| memPool = mem.DefaultBufferPool() | ||
| } | 
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.
Is it hard to fix the tests? Maybe we can do that as a follow-up.
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.
Acknowledged.
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.
Raised #8688 with the cleanup.
Co-authored-by: Doug Fawley <[email protected]>
8537f97    to
    e7b6d35      
    Compare
  
    
This change incorporates changes from golang/go#73560 to split reading HTTP/2 frame headers and payloads. If the frame is not a Data frame, it's read through the standard library framer as before. For Data frames, the payload is read directly into a buffer from the buffer pool to avoid copying it from the framer's buffer.
Testing
For 1 MB payloads, this results in ~4% improvement in throughput.
For smaller payloads, the difference in minor.
go run benchmark/benchmain/main.go -benchtime=60s -workloads=streaming \ -compression=off -maxConcurrentCalls=120 -trace=off \ -reqSizeBytes=100 -respSizeBytes=100 -networkMode=Local -resultFile="${RUN_NAME}" go run benchmark/benchresult/main.go streaming-before streaming-after Title Before After Percentage TotalOps 21490752 21477822 -0.06% SendOps 0 0 NaN% RecvOps 0 0 NaN% Bytes/op 1902.92 1902.94 0.00% Allocs/op 29.21 29.21 0.00% ReqT/op 286543360.00 286370960.00 -0.06% RespT/op 286543360.00 286370960.00 -0.06% 50th-Lat 352.505µs 352.247µs -0.07% 90th-Lat 433.446µs 434.907µs 0.34% 99th-Lat 536.445µs 539.759µs 0.62% Avg-Lat 333.403µs 333.457µs 0.02% GoVersion go1.24.7 go1.24.7 GrpcVersion 1.77.0-dev 1.77.0-devRELEASE NOTES: