tigerquoll commented on code in PR #1127:
URL: https://github.com/apache/yunikorn-core/pull/1127#discussion_r3888382428
##########
pkg/scheduler/objects/application.go:
##########
@@ -1162,7 +1187,26 @@ func (sa *Application) tryAllocate(headRoom
*resources.Resource, allowPreemption
// because the len check above guarantees at least one iteration would
occur.
backoffThreshold := sa.queue.GetMaxAppUnschedAskBackoff()
// get all the requests from the app sorted in order
- for _, request := range sa.sortedRequests {
+ // LOAD-BEARING INVARIANT: sa.sortedRequests now holds only pending
asks and
+ // allocateAsk/deallocateAsk mutate it in place. An index based loop is
used deliberately: it
+ // re-reads len() and re-indexes the current slice on every iteration,
so a mid-loop remove can
+ // never make it read the nil'd tail slot, and it stays correct if an
insert reallocates the
+ // backing array. A "range" loop captures the slice header once and
would panic in that case.
+ // On top of that the length is asserted below: every
successful-allocation path returns
+ // immediately today, so a length change mid-loop means a future edit
mutated and continued.
+ // Worst case without the assert is skipping a single ask for this
cycle (self-correcting on the
+ // next cycle) rather than a crash. If a mutate-then-continue is ever
really needed, restructure
+ // to a snapshot copy (see tryPlaceholderAllocate).
+ startLen := len(sa.sortedRequests)
+ for i := 0; i < len(sa.sortedRequests); i++ {
+ if len(sa.sortedRequests) != startLen {
+ log.Log(log.SchedApplication).DPanic("sortedRequests
mutated during tryAllocate iteration",
+ zap.String("application ID", sa.ApplicationID),
+ zap.Int("length at loop start", startLen),
+ zap.Int("current length",
len(sa.sortedRequests)))
+ return nil
+ }
+ request := sa.sortedRequests[i]
Review Comment:
Agreed the length assert cannot fire — dropped it together with the comment
block. One clarification for the record: the hazard the loop guarded was
same-goroutine, not concurrent access — `allocateAsk` is called from inside the
loop and now removes from the slice being iterated. Every allocating path
returns, so that hazard is equally dead today.
I measured performance on the clone approach -
`BenchmarkScheduling/1000Nodes/10000Pods`, 3 alternating samples, medians:
| loop | pods/s | B/op |
|---|---:|---:|
| index loop | 38,604 | 203 MB |
| `slices.Clone` | 34,461 | 407 MB |
The clone is O(pending) per call for a loop that normally exits at index 0,
so it costs 11% of throughput and doubles allocation on this workload. Can I
suggest we keep the plain index loop (it re-reads `len()`, so the ghost repair
below can remove and continue) with a three-line comment and no assert. If you
would still rather have the clone for uniformity it is a two-line change — but
I would prefer not to hand back 11% of what this PR buys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
