wilfred-s commented on code in PR #1127:
URL: https://github.com/apache/yunikorn-core/pull/1127#discussion_r4152117901
##########
pkg/scheduler/objects/application.go:
##########
@@ -1162,7 +1180,14 @@ 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 {
+ // allocateAsk drops the ask from sa.sortedRequests and every path that
allocates returns, so the
+ // only removal this loop ever observes is the ghost repair below. The
index based loop re-reads
+ // len() and re-indexes the current slice on every iteration, which is
what makes that
+ // remove-and-continue safe. Iterating a slices.Clone snapshot instead
(as tryPlaceholderAllocate
+ // has to) measured -11% pods/s and 2x bytes/op on BenchmarkScheduling:
it is O(pending) per call
+ // for a loop that normally exits at index 0.
+ for i := 0; i < len(sa.sortedRequests); i++ {
+ request := sa.sortedRequests[i]
Review Comment:
I am still a hard -1 on this.
Even if the clone is sightly slower. The simplicity of using the clone and
maintainability of the solution is worth that.
--
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]
