Repository navigation
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the itemList queue in outStream with an intrusive singly linked list of dataFrames (dataHead and dataTail) to avoid allocating a separate queue per stream. It also introduces a trailer field to outStream to store server trailers, which are processed after all data frames are written. The loopyWriter methods have been updated to utilize these new fields, and a new unit test TestOutStream_DataQueue has been added to verify the correctness of the intrusive queue operations. There are no review comments, and we have no further feedback to provide.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9490 +/- ##
==========================================
- Coverage 87.71% 87.60% -0.12%
==========================================
Files 436 436
Lines 30822 30837 +15
==========================================
- Hits 27036 27015 -21
- Misses 3785 3822 +37
+ Partials 1 0 -1
🚀 New features to boost your workflow:
|
#9444 replaced the linked lists in the control buffer and in each stream's queue of data frames with ring buffers. This works well for the control buffer, which is long lived and shared by all streams. For per-stream queues, however, the ring buffer allocates a backing array on each stream's first enqueue, which costs one allocation per stream on both the client and the server. This buffer can also pin up to 1KB memory per stream.
This PR replaces the per-stream ring buffer with an intrusive singly linked list:
dataFramegets anextfield andoutStreamkeepsdataHead/dataTailpointers. AdataFrameis only ever queued on one stream, and the queue is only accessed by the loopy writer, so no extra synchronization is needed. Adding the field does not change the allocation size class ofdataFrame(72 → 80 bytes).Server trailers, which were previously queued behind the data frames as a
*serverHeadersitem, are now stored in a separateoutStream.trailerfield and written once the data queue is empty. As a result, the queue only holds*dataFramesand the type assertions inprocessDataandupdateStreamAfterWriteare removed.The control buffer continues to use the ring buffer from #9444.
Benchmarks
benchmain with -bufconn -networkMode=Local -compression=off -benchtime=60s.
Unary, 100 concurrent calls, 0-byte payloads
The 2 fewer allocations per RPC are the per-stream ring buffer arrays (one on the client, one on the server).
Streaming, 200 concurrent calls, 100-byte payloads
Streams are long lived in this workload, so the ring buffer's per-stream allocation was already negligible.
RELEASE NOTES: N/A