[DNM] Revert "client: clean reference in priority queue (#1835)" #1844
Conversation
This reverts commit 01cdf70.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
the
dco-signoff: yes
📝 WalkthroughWalkthroughThe changes refactor the priority queue's Changes
Sequence DiagramsequenceDiagram
participant CB as ClientBatch
participant PQ as PriorityQueue
participant Items as Items
loop Batching Loop
CB->>PQ: Take(n)
PQ-->>CB: []Item (reqs)
alt reqs is empty
CB-->>CB: break
else reqs has items
CB->>Items: build(reqs)
Items-->>CB: processed
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
Comment |
ti-chi-bot
bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/client/priority_queue.go (1)
56-63: Memory leak: removed reference must be cleared.The
Popmethod no longer clears the reference to the removed item in the backing array. When a slice is shrunk viaold[0:n-1], the element at indexn-1remains in the backing array's capacity and prevents GC from collecting the referenced object.This is the same issue explicitly documented in
client_batch.go(lines 216-219) regardingmemdbmemory leaks in large transactions.Proposed fix
func (ps *prioritySlice) Pop() interface{} { old := *ps n := len(old) item := old[n-1] + old[n-1] = nil // avoid memory leak *ps = old[0 : n-1] return item }internal/client/client_batch.go (1)
214-229: Existing comment documents the memory leak being reintroduced.The comment at lines 216-219 explicitly warns about the memory leak issue with backing array references:
"The data in the cap part of the slice would reference the prewrite keys whose underlying memory is borrowed from memdb. The reference cause GC can't release the memdb, leading to serious memory leak problems in the large transaction case."
This is the exact issue that the reverted PR
#1835was fixing inpriority_queue.go. TheTake()method now exhibits the same problematic pattern whenn >= Len()by usingpq.ps[:0]without clearing references.
🤖 Fix all issues with AI agents
In `@internal/client/priority_queue.go`:
- Around line 99-102: The branch that returns all items when n >= pq.Len()
returns pq.ps and then reslices pq.ps to pq.ps[:0] without niling elements,
leaking references; update the code in the block that handles this case (the
return-all branch using pq.ps and pq.Len()) to either copy the returned slice
into a new slice (so you can safely clear/nil the original backing array) or
explicitly nil out each element of pq.ps before reslicing (mirroring the
behavior in reset()), ensuring you clear references on pq.ps after obtaining ret
so GC can reclaim the items.
🧹 Nitpick comments (1)
internal/client/priority_queue_test.go (1)
37-73: Missing test coverage for backing-array reference clearing.According to the summary,
TestPriorityQueueTakeAllLeavesReferencesInBackingArraywas removed. This test was verifying that references are properly cleared in the backing array to prevent memory leaks. Without this test, future regressions in reference clearing behavior won't be caught.Consider adding a test that verifies items can be garbage collected after being taken from the queue.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/client/client_batch.gointernal/client/priority_queue.gointernal/client/priority_queue_test.go
⏰ 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). (8)
- GitHub Check: race-test
- GitHub Check: test
- GitHub Check: integration-local
- GitHub Check: integration-tikv
- GitHub Check: integration-raw-tikv (v1ttl)
- GitHub Check: integration-local-race
- GitHub Check: integration-next-gen-tikv
- GitHub Check: integration-raw-tikv (v2)
🔇 Additional comments (3)
internal/client/client_batch.go (1)
188-192: Correct adaptation to the new Take() API.The two-step pattern (take then check) properly handles the case when the queue is empty or all items are canceled. The
len(reqs) == 0guard prevents callingbuildwith an empty slice.internal/client/priority_queue_test.go (1)
48-64: Test correctly updated to use the new return-based API.The test logic remains sound, verifying:
Take(1)returns highest priority itemTake(2)returns next two in priority orderTake(5)when only 2 items remain returns those 2- Queue state (length, highest priority) is correctly updated
internal/client/priority_queue.go (1)
94-110: API change is correctly implemented across the codebase.The signature change from callback-based to return-based is cleaner and correctly implemented. The single caller in
internal/client/client_batch.go:188has been properly updated to use the new return-based API (reqs := b.entries.Take(int(n))), and the subsequent code correctly handles the return value by checkinglen(reqs) == 0. Then <= 0returningnilis a reasonable sentinel value, though this edge case is not explicitly tested in the test suite.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if n >= pq.Len() { | ||
| ret := pq.ps | ||
| pq.ps = pq.ps[:0] | ||
| cb(ret) | ||
| for i := range ret { | ||
| ret[i] = nil | ||
| } | ||
| return | ||
| return ret |
There was a problem hiding this comment.
Memory leak: references not cleared when taking all items.
When n >= pq.Len(), the code returns the slice and resets length to 0 via pq.ps[:0], but the backing array still holds references to all items. This prevents GC from collecting those items.
This is inconsistent with the reset() method (lines 140-145) which correctly nils out all elements before reslicing. The same pattern should be applied here.
Proposed fix
if n >= pq.Len() {
ret := pq.ps
+ for i := 0; i < len(pq.ps); i++ {
+ pq.ps[i] = nil
+ }
pq.ps = pq.ps[:0]
return ret
}Note: This requires making a copy of ret before clearing, or clearing after returning a copy:
if n >= pq.Len() {
- ret := pq.ps
- pq.ps = pq.ps[:0]
- return ret
+ ret := make([]Item, len(pq.ps))
+ copy(ret, pq.ps)
+ pq.reset()
+ return ret
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if n >= pq.Len() { | |
| ret := pq.ps | |
| pq.ps = pq.ps[:0] | |
| cb(ret) | |
| for i := range ret { | |
| ret[i] = nil | |
| } | |
| return | |
| return ret | |
| if n >= pq.Len() { | |
| ret := pq.ps | |
| for i := 0; i < len(pq.ps); i++ { | |
| pq.ps[i] = nil | |
| } | |
| pq.ps = pq.ps[:0] | |
| return ret | |
| } |
🤖 Prompt for AI Agents
In `@internal/client/priority_queue.go` around lines 99 - 102, The branch that
returns all items when n >= pq.Len() returns pq.ps and then reslices pq.ps to
pq.ps[:0] without niling elements, leaking references; update the code in the
block that handles this case (the return-all branch using pq.ps and pq.Len()) to
either copy the returned slice into a new slice (so you can safely clear/nil the
original backing array) or explicitly nil out each element of pq.ps before
reslicing (mirroring the behavior in reset()), ensuring you clear references on
pq.ps after obtaining ret so GC can reclaim the items.
1 participant
DO NOT MERGE IT
This PR is used to test the AI's ability.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.