tigerquoll commented on code in PR #1127:
URL: https://github.com/apache/yunikorn-core/pull/1127#discussion_r3888396204
##########
pkg/scheduler/objects/application.go:
##########
@@ -1358,7 +1412,11 @@ func (sa *Application)
tryPlaceholderAllocate(nodeIterator func() NodeIterator,
var phFit *Allocation
var reqFit *Allocation
// get all the requests from the app sorted in order
- for _, request := range sa.sortedRequests {
+ // NOTE: iterate over a snapshot, not sa.sortedRequests directly. The
revert path below calls
+ // sa.deallocateAsk(request), which re-inserts the ask into
sa.sortedRequests mid-range - mutating
+ // the live slice while ranging over it would skip/revisit entries.
allocateAsk/deallocateAsk keep
+ // only pending asks in sortedRequests.
+ for _, request := range slices.Clone(sa.sortedRequests) {
Review Comment:
To show the working on why the two loops differ:
`tryAllocate` never continues past a mutation: every path that allocates
returns (`tryNode` always returns a result; preemption only reserves). The only
remove-and-continue is the ghost repair, which an index loop handles by
re-reading `len()`. The loop normally exits at index 0, so a clone there adds
an O(pending) copy to an O(1) loop — that is the −11%.
`tryPlaceholderAllocate` does mutate and keep going: the revert path is
`allocateAsk` (removes) → `SetReleased` fails → `deallocateAsk` (`reinsert`s at
the head of the tie-group) → `continue`. An index loop would still be correct
today, but only because `reinsert` always lands at or before the current index
— a property of `LessThan`'s tie handling. A snapshot removes that dependency,
which is what your advice is for. And this loop already scans the whole slice
looking for task-group asks that match a placeholder, so the copy is a constant
factor on a linear loop, not a new term — nothing to give back.
Same principle both places: pick the iteration form that is robust for what
the loop does to the slice, and pay for a snapshot only where the loop walks
the whole slice anyway.
--
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]
