tigerquoll commented on code in PR #1127:
URL: https://github.com/apache/yunikorn-core/pull/1127#discussion_r3888404440
##########
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:
Correction to my note above, after re-checking against the code:
`tryPlaceholderAllocate` does not scan the whole slice - it returns on the
first successful replacement, so it exits early just as `tryAllocate` does, and
the clone there is an O(pending) copy per call as well. What differs is the
population (only applications currently holding placeholders, during
replacement) and that the snapshot is what makes the mutate-and-continue revert
correct by construction rather than by the tie-handling property. I have not
measured that cost - `BenchmarkScheduling` has no gang workload. If it ever
matters, an index loop is safe there today for the reason above, but where the
loop genuinely mutates and continues I would rather keep the snapshot.
Also re-measured the `tryAllocate` comparison on the rebased head (b0a2830
against the same with `slices.Clone`), same method: index loop 38,041 pods/s /
205 MB/op, clone 33,431 pods/s / 410 MB/op — −12%. The earlier figures were
taken on the pre-rebase base and stand.
--
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]
