Skip to content

Commit c29c8d3

Browse files
committed
refactor(flowcontrol): apply review feedback on the priority queue
Addresses review comments from @shmuelk on llm-d#1690: - Correct the new-file license headers to "2026 The llm-d Authors". - Clarify the PriorityQueue doc: ordering is maintained by an internal container/heap (the queue does not itself implement heap.Interface). - Narrow the Add and Remove critical sections to the shared heap mutation. Precondition checks (nil, type assertion) and the atomic byteSize update move outside the lock; the isInvalidated and index reads stay inside it, since those fields are written under the lock by Remove/Cleanup/Drain and reading them lock-free would race. - Compact Cleanup survivors in place instead of allocating a second slice, clearing the vacated tail so removed items are not retained. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
1 parent 4ac5b22 commit c29c8d3

2 files changed

Lines changed: 29 additions & 19 deletions

File tree

pkg/epp/flowcontrol/framework/plugins/queue/priorityqueue.go

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright 2025 The Kubernetes Authors.
2+
Copyright 2026 The llm-d Authors.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.
@@ -27,7 +27,7 @@ import (
2727

2828
// PriorityQueueName is the name of the priority queue implementation.
2929
//
30-
// This queue provides a concurrent-safe priority queue backed by the standard library's
30+
// This queue provides a concurrent-safe priority queue whose ordering is maintained by an internal
3131
// container/heap. Items are ordered by the configured OrderingPolicy, with the highest-priority
3232
// item (per the policy) at the head. It advertises the CapabilityPriorityConfigurable capability.
3333
//
@@ -157,30 +157,34 @@ func (pq *priorityQueue) Peek() flowcontrol.QueueItemAccessor {
157157
// Add adds an item to the queue.
158158
// Time complexity: O(log n).
159159
func (pq *priorityQueue) Add(item flowcontrol.QueueItemAccessor) {
160-
pq.mu.Lock()
161-
defer pq.mu.Unlock()
162-
163160
hi := &heapItem{item: item}
164161
item.SetHandle(hi)
162+
163+
pq.mu.Lock()
165164
heap.Push(pq.heap, hi)
165+
pq.mu.Unlock()
166+
166167
pq.byteSize.Add(item.OriginalRequest().ByteSize())
167168
}
168169

169170
// Remove removes an item from the queue.
170171
// Time complexity: O(log n).
171172
func (pq *priorityQueue) Remove(handle flowcontrol.QueueItemHandle) (flowcontrol.QueueItemAccessor, error) {
172-
pq.mu.Lock()
173-
defer pq.mu.Unlock()
174-
175-
if handle == nil || handle.IsInvalidated() {
173+
if handle == nil {
176174
return nil, contracts.ErrInvalidQueueItemHandle
177175
}
178-
179176
hi, ok := handle.(*heapItem)
180177
if !ok {
181178
return nil, contracts.ErrInvalidQueueItemHandle
182179
}
183180

181+
pq.mu.Lock()
182+
defer pq.mu.Unlock()
183+
184+
if hi.IsInvalidated() {
185+
return nil, contracts.ErrInvalidQueueItemHandle
186+
}
187+
184188
// Validate membership by identity: a *heapItem is created in Add and only ever lives in a single
185189
// queue's slice, so a matching pointer at its tracked index proves it belongs to this queue and
186190
// is still present. This also guards against a stale index (e.g., the item was concurrently
@@ -192,7 +196,7 @@ func (pq *priorityQueue) Remove(handle flowcontrol.QueueItemHandle) (flowcontrol
192196

193197
heap.Remove(pq.heap, i)
194198
pq.byteSize.Add(^hi.item.OriginalRequest().ByteSize() + 1) // Atomic subtraction.
195-
handle.Invalidate()
199+
hi.Invalidate()
196200
return hi.item, nil
197201
}
198202

@@ -202,24 +206,30 @@ func (pq *priorityQueue) Cleanup(predicate contracts.PredicateFunc) []flowcontro
202206
defer pq.mu.Unlock()
203207

204208
var removedItems []flowcontrol.QueueItemAccessor
205-
itemsToKeep := make([]*heapItem, 0, len(pq.heap.items))
206209

207-
for _, hi := range pq.heap.items {
210+
// Compact survivors in place: the kept count never exceeds the read index, so survivors can be
211+
// written back into the existing backing array instead of allocating a second slice.
212+
items := pq.heap.items
213+
kept := 0
214+
for _, hi := range items {
208215
if predicate(hi.item) {
209216
removedItems = append(removedItems, hi.item)
210217
hi.Invalidate()
211218
hi.index = -1
212219
pq.byteSize.Add(^hi.item.OriginalRequest().ByteSize() + 1) // Atomic subtraction.
213-
} else {
214-
itemsToKeep = append(itemsToKeep, hi)
220+
continue
215221
}
222+
items[kept] = hi
223+
hi.index = kept
224+
kept++
216225
}
217226

218227
if len(removedItems) > 0 {
219-
for i, hi := range itemsToKeep {
220-
hi.index = i
228+
// Clear the vacated tail so removed items aren't retained by the backing array.
229+
for i := kept; i < len(items); i++ {
230+
items[i] = nil
221231
}
222-
pq.heap.items = itemsToKeep
232+
pq.heap.items = items[:kept]
223233
// Re-establish the heap property on the remaining items.
224234
heap.Init(pq.heap)
225235
}

pkg/epp/flowcontrol/framework/plugins/queue/priorityqueue_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright 2025 The Kubernetes Authors.
2+
Copyright 2026 The llm-d Authors.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.

0 commit comments

Comments
 (0)