Only use opened queue slots on running workers #7190
Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ± 0 15 suites ±0 6h 52m 44s ⏱️ + 18m 17s For more details on these failures, see this check. Results for commit 29cf615. ± Comparison against base commit 0983731. ♻️ This comment has been updated with latest results. |
| if state.queued and not _worker_full(ws, state.WORKER_SATURATION): | ||
| if ( | ||
| state.queued | ||
| and ws.status == Status.running |
There was a problem hiding this comment.
This is true right now but if we ever had more Status types, of which some might actually be runnable, this could cause a deadlock, couldn't it?
I know this is pretty hypothetical but my point is that we do not change the behavior but encode more assumptions in the code for a perf improvement you call yourself minuscule (and rare)
I don't mean to block this if you consider it truly better readable but I'm a soft -0
There was a problem hiding this comment.
That seems like a pretty far-fetched problem. There are already 18 instances of == Status.running in scheduler.py.
I've added a test. This is still not a big deal, it's just a small tidying up.
|
The primary change here was incorporated in #7224 |
2 participants
Minor readability and performance improvement that avoids trying to send a queued task to processing when a slot opens on a non-running worker.
This doesn't affect correctness. Currently, a task will be recommended to
processingand we'll look for a worker for it. Since the worker isn't running, we won't pick it, and the task will stay inqueued. With this PR, we simply save ourselves doing the search when we know it won't be fruitful.I haven't come up with a nice way to test this (since it shouldn't affect behavior and the performance difference will be minuscule anyway.) I could look at the scheduler transition logs and assert there's no extra queued->processing->queued transition in this case, but that feels a bit nitpicky for a case that isn't very important.
pre-commit run --all-files