[SPARK-43086][CORE] Add configurable bin-packing task placement#57484
[SPARK-43086][CORE] Add configurable bin-packing task placement#57484starcatmeow wants to merge 1 commit into
Conversation
Introduce spark.scheduler.taskPlacement.strategy with SPREAD as the default and BIN_PACK as an opt-in strategy. Preserve the PROCESS_LOCAL, NODE_LOCAL, and RACK_LOCAL scheduling passes, then pack executor offers during the NO_PREF and ANY passes by preferring executors with running or current-round assigned tasks before using lexicographic executor ID order. Add coverage for configuration validation, spread and bin-pack placement, locality, multiple TaskSets, custom resources, barrier tasks, and provisional barrier assignments.
e2c2dd8 to
56b0c6a
Compare
viirya
left a comment
There was a problem hiding this comment.
I traced the scheduler paths independently and this holds up — including a few spots worth spelling out since it's a placement change.
- SPREAD is behavior-preserving. With
useBinPacking = falsethe new innerdo { ... } while (useBinPacking && launchedTask)runs the body exactly once per offer index, soresourceOfferSingleTaskSetstill does a single pass launching at most one task per offer, and the outerdo/while(launchedTaskAtCurrentMaxLocality)still drives the round-robin. Default behavior is unchanged. - The busy/idle predicate is well chosen.
isExecutorBusy(execId) || availableCpus(i) < shuffledOffers(i).cores— the first term catches executors made busy by prior rounds or earlieraddRunningTasks in this round, and the second catches provisional barrier assignments in the current round (which decrementavailableCpusbut aren't registered as running until the whole barrier set launches). The comment captures this well, and the "treats barrier locality assignments as busy" test pins it. - Shared index space stays intact. Only the iteration order is reordered via
offerIndicesByExecutorId;shuffledOffers/availableCpus/availableResources/tasksare all still addressed by the original indexi, so there's no re-indexing hazard, and the sort happens once perresourceOfferscall. - Delay-scheduling accounting is unaffected.
hasScheduleDelayRejectonly fires atmaxLocality == ANYwith pending-but-unschedulable tasks. BIN_PACK just moves the repeated probing of a single offer from the outer loop into the inner loop; the final no-launch probe is what can set the reject in both cases, sonoDelayScheduleRejectsreaches the same result. - Locality is preserved — BIN_PACK only applies at NO_PREF/ANY, and the PROCESS_LOCAL/NODE_LOCAL tests confirm it.
Two non-blocking notes:
- Executor IDs are sorted lexicographically (
sortBy(_.executorId)on the string), so e.g."10"sorts before"2". That's fine for correctness — bin-packing only needs a stable order — and the doc does say "lexicographic executor ID order," so it's deliberate. Just flagging that since executor IDs are numeric-looking, the ordering can read as surprising; a half-sentence noting it's lexicographic purely for determinism would help the next reader. - Under BIN_PACK the inner loop already fills each offer to exhaustion, so the enclosing
do/while(launchedTaskAtCurrentMaxLocality)will make one extra scan that launches nothing before exiting. Harmless, just a minor redundant pass — not worth changing.
Docs and test coverage are thorough. LGTM.
|
this one looks very similar to #56957 |
|
So the motivation seems to make auto-scale more efficient? Can we have a proper design discussion for the end to end auto-scale workflow? Why changing the scheduler is the best design choice? Just for example, one idea can be: when the auto scaler predicate an upcoming idle time window, it can pick some executors and mark them as decommissioned, then scheduler will not assign new tasks to these executors, and auto scaler can kill them after current tasks are done. |
|
@pan3793 Interesting! I didn’t realize there was already another PR solving the same problem. It probably makes sense to consolidate the efforts rather than have two similar implementations. WDYT, @starcatmeow? @cloud-fan Yes, the main motivation is to make dynamic allocation more efficient. Bin-packing seemed like a relatively straightforward way to do that while building on Spark’s existing scheduling and scale-down mechanisms.
That’s an interesting idea. One question I have is how reliably Spark can predict future executor demand or an upcoming idle window. If demand picks up again shortly afterward, prematurely decommissioning executors could cause unnecessary churn. I also see bin-packing as complementary to that approach: it concentrates tasks on fewer executors, leaving the others idle and creating more opportunities for scale-down. The autoscaler can then decide whether to remove those executors using its existing idle timeout or, in the future, a more demand-aware policy. Happy to have more design discussions and hear more ideas on making auto-scaling better! |
|
@viirya Thanks for the thorough review and for checking these edge cases. Both non-blocking notes make sense. Since we may consolidate with #56957, I’ll keep them in mind for whichever implementation becomes the base. Thanks @pan3793 for pointing out #56957, and thanks everyone for the discussion. I reviewed #56957, and I agree that the two PRs overlap substantially and should be consolidated rather than maintained as two independent implementations. The main differences I see are:
I do not have a strong preference about which PR should be the base. If the maintainers prefer the more general assignment-strategy abstraction, I am happy to collaborate on #56957, port over any useful busy-state handling and test @cloud-fan Happy to have a broader design discussion! I don’t want to claim that changing the scheduler is necessarily the best or only approach. |
|
Thanks, @starcatmeow. Yes, when it comes to duplicate or similar PRs, Apache Spark goes by a first-come, first-served rule. Given the code content of both PRs, I'd recommend to proceed @ulysses-you 's PR first. |
|
Thanks for clarifying. I’ll close this PR in favor of #56957, happy to collaborate there. Thanks everyone for the reviews and discussion! |
What changes were proposed in this pull request?
This PR adds an opt-in bin-packing task placement strategy to
TaskSchedulerImpl.It introduces
spark.scheduler.taskPlacement.strategywith two values:SPREAD, the default, preserves the existing behavior of cycling through shuffledexecutor offers one task at a time.
BIN_PACKfills an eligible executor before moving to the next one.BIN_PACKapplies only during theNO_PREFandANYscheduling passes.PROCESS_LOCAL,NODE_LOCAL, andRACK_LOCALretain Spark's existing shuffled offerorder, so locality continues to take precedence over packing.
For bin-packed passes, executors with running tasks or tasks assigned earlier in the
current
resourceOfferscall are considered before idle executors. Each group is orderedlexicographically by executor ID. The scheduler sorts offer indices once per
resourceOfferscall, leavingshuffledOffers,availableCpus,availableResources,and the output task buffers in their existing shared index space. It then performs a
stable busy/idle partition for each TaskSet and bin-packed pass so that later TaskSets
observe assignments made earlier in the scheduling round.
The PR also documents the new configuration and adds tests for the configuration,
placement order, locality, multiple TaskSets, custom resources, and barrier tasks.
Why are the changes needed?
SPARK-43086 describes a resource
efficiency issue when dynamic allocation is used with the existing spread placement
behavior.
Dynamic allocation removes an executor only after it becomes idle. When a stage has
fewer tasks than the available cluster slots, spreading those tasks across executors can
keep more executors busy than necessary and delay scale-down.
For example, if four executors have two task slots each, a four-task stage can place one
task on each of four executors with
SPREAD. WithBIN_PACK, it can instead place twotasks on each of two executors, allowing the other two executors to become idle and be
removed.
The new strategy is opt-in to avoid changing Spark's existing default placement and
latency behavior.
Does this PR introduce any user-facing change?
Yes. It adds the following configuration:
spark.scheduler.taskPlacement.strategy=BIN_PACKSupported values are:
SPREAD: preserves the existing placement behavior and remains the default.BIN_PACK: fills busy executors first and then idle executors, using lexicographicexecutor ID order within each group, during the
NO_PREFandANYpasses.Applications that do not set this configuration retain the existing behavior.
Locality-specific placement for
PROCESS_LOCAL,NODE_LOCAL, andRACK_LOCALisunchanged.
How was this patch tested?
The scheduler unit suite passed:
build/sbt 'core/testOnly org.apache.spark.scheduler.TaskSchedulerImplSuite'All 122 tests passed. The added coverage includes:
SPREADbehavior;BIN_PACKexecutor ordering;PROCESS_LOCALandNODE_LOCALplacement;ANYlocality;Scalastyle and a full package build also passed:
The two strategies were additionally tested with real executor JVMs using:
local-cluster[4,2,1024];NO_PREFtasks;The steady executor counts observed during each stage were:
SPREADBIN_PACKThe Spark Event Timeline showed that
SPREADfirst removed executors during thetwo-task stage, while
BIN_PACKremoved two executors during the four-task stage andanother executor during the two-task stage.
SPREAD:BIN_PACK:Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex (GPT-5)