client: pre-encode request before sending to batch-send-loop #1841
Conversation
Signed-off-by: zyguan <zhongyangguan@gmail.com>
ti-chi-bot
bot
added
dco-signoff: yes
Signed-off-by: zyguan <zhongyangguan@gmail.com>
Signed-off-by: zyguan <zhongyangguan@gmail.com>
ti-chi-bot
bot
added
size/XL
| return nil | ||
| } | ||
| data := globalEncodedMsgDataPool.Get(req.Cmd.Size()) | ||
| n, err := req.Cmd.MarshalTo(data) |
There was a problem hiding this comment.
is it possible that req.Cmd could being modified concurrently by other goroutines?
maybe we should add some comments for this function for example :
// encodeRequestCmd encodes req.Cmd. The caller must ensure that req is not
// being modified concurrently by other goroutines.
There was a problem hiding this comment.
No, encodeRequestCmd is used in batch client internally. It's called just after tikvrpc.Request.ToBatchCommandsRequest() and before constructing batchCommandsEntry. Note that the req here is the output of ToBatchCommandsRequest, which is a new object of tikvpb.BatchCommandsRequest_Request. So req.Cmd can be only modified inside the batch client by reuseRequestData after the req has been sent. No data race on req.Cmd via public API (SendRequest and SendRequestAsync).
ti-chi-bot
bot
added
needs-1-more-lgtm
|
|
||
| if err := encodeRequestCmd(batchReq); err != nil { | ||
| cb.Invoke(nil, err) | ||
| return |
There was a problem hiding this comment.
It can early-return via cb.Invoke before entry.cb.Inject(...) is installed at L105, would it be a problem that regionRPC.End()/spanRPC.Finish() won’t run on this path or should we consider move encoding after the inject setup?
There was a problem hiding this comment.
I think it's OK, if it early-returned, there is no RPC actually, besides, this function shouldn't return error in most of cases.
internal/client/client_batch.go
Outdated
| var globalEncodedMsgDataPool grpc.SharedBufferPool | ||
|
|
||
| func init() { | ||
| globalEncodedMsgDataPool = grpc.NewSharedBufferPool() |
There was a problem hiding this comment.
Is this a experimental API?
There was a problem hiding this comment.
Yes, and it has been moved to mem package in new version.
| globalEncodedMsgDataPool = grpc.NewSharedBufferPool() | ||
| } | ||
|
|
||
| type encodedBatchCmd struct { |
There was a problem hiding this comment.
How about implementing isBatchCommandsRequest_Request_Cmd directly to avoid embedding.
There was a problem hiding this comment.
Unfortunately, you cannot implement an unexported interface method from a different package in golang.
There was a problem hiding this comment.
We may implement it in kvproto later.
Signed-off-by: zyguan <zhongyangguan@gmail.com>
📝 WalkthroughWalkthroughThis pull request introduces a batch request pre-encoding mechanism with object pooling. It adds an early encoding step to the async batch request flow that converts regular command objects to pre-encoded batch command objects, which are reused from a global pool to reduce per-request memory allocations and ensure validation prior to processing. Changes
Sequence Diagram(s)sequenceDiagram
participant Async as Async Client
participant Encoder as encodeRequestCmd
participant Pool as Encoded Pool
participant Batch as Batch Processor
Async->>Encoder: Convert Cmd to encodedBatchCmd
Encoder->>Pool: Acquire pre-encoded data buffer
Pool-->>Encoder: Return buffer (or error if exhausted)
Encoder->>Encoder: Validate & set encoding state
Encoder-->>Async: Return encodedBatchCmd or error
alt Encoding Success
Async->>Batch: Send batch with pre-encoded command
Batch->>Batch: Marshal and process
Batch->>Pool: Return encoded data (reuseRequestData)
else Encoding Error
Async->>Async: Invoke callback with error & return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code graph analysis (1)internal/client/client_test.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (18)
✏️ Tip: You can disable this entire section by setting Comment |
|
/retest |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AndreMouche, cfzjywxk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/cherry-pick tidb-8.5 |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
@zyguan: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
Signed-off-by: zyguan <zhongyangguan@gmail.com>
4 participants
This PR tries to prevent issues like #1839 by pre-encoding request.
The micro-bench results show that the overhead introduced by this PR is negligible. Besides, we can reduce the mem allocs of
AttachContextafter this PR.Summary by CodeRabbit
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.