Repository navigation
Conversation
…cutor Every task that read a Comet broadcast decompressed and decoded the whole broadcast for itself. DecodedBroadcastCache lets the tasks of an executor that read a broadcast at the same time share one decoded copy: the first decodes it into buffers the cache owns, tasks that arrive during that decode wait for it, and each reads vectors of its own over the shared buffers. An entry is freed when the last task holding it completes. A failed decode is not cached, and a broadcast with dictionaries is still decoded per task. CometBatchRDD reads through the cache, so every broadcast consumer uses it. spark.comet.exec.broadcastExchange.shareDecoded.enabled turns it off. The broadcast decode and the driver-side coalescing also read the IPC stream through a channel that reads up to 64 KiB at a time, instead of Channels.newChannel, which reads 8 KiB at a time.
Member
Author
|
@comphead not sure if this overlaps with any of your PRs? Happy to close this one if it does |
The cache holds no memory the tasks would not hold anyway only for a broadcast of one batch, which a native join keeps for as long as its task runs. List the cache's child allocator in the memory management guide, say that the read channel copies through a reused array, count holds per read rather than per task, and drop an unused accessor.
… decode its task outlived Arrow reads a whole message body with one read, which for a coalesced broadcast is all of it, so the read channel now checks for an interrupt before each piece, as Channels.newChannel did, instead of once per read. A task whose completion listener runs on another thread can give back its hold while its read decodes the broadcast. If it was the last holder, the entry was freed before the decode finished, and the decode was kept where nothing would free it. The decode is now freed instead, and the read decodes the broadcast for itself.
The shared batch iterator now keeps a single iterator, of views or of the task's own decode, instead of branching on which one it has. A decode that keeps nothing frees its batches through Decoded.close. The four tests that block a decode share one cache, ScalaTest's eventually replaces a hand-written wait, UtilsSuite's new tests reuse its serialize helper, and the test of a stream larger than one read piece checks against MaxReadPiece.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #6806. Part of #6013.
Rationale for this change
Every task that reads a Comet broadcast decompresses and decodes the whole broadcast for itself, in
CometBatchRDD.compute. Spark keeps one copy of a broadcast's value per executor, but Comet's value is Arrow IPC compressed with Spark's codec, so each probe task of a broadcast hash join repeats the same decode. #6806 profiled a broadcast hash join with a 1M-row build side: about three quarters of each Comet probe task went to that decode, against 6% for building the hash table.What changes are included in this PR?
DecodedBroadcastCachelets the tasks of an executor that read a broadcast at the same time share one decoded copy of it. The first task to read a broadcast value decodes it into buffers the cache owns. Tasks that arrive during that decode wait for it, and check every 100 ms whether they have been killed. Each task then reads vectors of its own that hold references on the shared buffers, so the native input reader can close what it reads, as it does today, without affecting other tasks.coalesceBroadcastBatchesleaves as it found them, is still decoded by each task.CometBatchRDD.computereads through the cache, so it covers every consumer of a Comet broadcast: broadcast hash joins of every join type, broadcast nested loop joins, and JVM consumers.spark.comet.exec.broadcastExchange.shareDecoded.enabled, true by default, turns it off. The tuning guide describes it, and the memory management guide lists the cache's allocator.Utils.decodeBatches, the cache and the driver-side coalescing read the IPC stream through a channel that fills the destination buffer up to 64 KiB at a time, instead ofChannels.newChannel, which reads 8 KiB at a time through the JDK's interruptible I/O machinery. Like that channel, it fails withClosedByInterruptExceptionbefore its next piece once its thread is interrupted.Each task still builds its own hash table. Sharing that is the rest of #6013 and needs apache/datafusion#25491, which is in DataFusion 56. The broadcast also stays compressed twice (#2216). Comet's codec layer is now undone once per executor while its tasks overlap, rather than once per task.
How are these changes tested?
DecodedBroadcastCacheSuiteruns each case through a cache whose decode the test can block or fail:CometBatchRDDgoes through the cache only when the setting is onUtilsSuiteadds a stream larger than one read piece, a decode on an interrupted thread, and a read that stops at its next piece once its thread is interrupted.Each of ten mutations fails at least one of these tests: no sharing, no retry after a failed decode, no kill check, a task's last view not closed when it completes, a view not closed when the reader moves on, no dictionary check,
CometBatchRDDignoring the setting, entries never freed, a decode kept after every task holding its entry completed, and an interrupt checked only when a read starts.CometJoinSuite,CometExecSuite,CometArrowStreamSuite,CometExecIteratorLifecycleSuite,CometDppFallbackRepro3949Suite,UtilsSuiteand the new suite pass on Spark 4.1 (342 tests), and strict Scala warnings pass on Spark 3.5. Two local stress runs that are not part of this PR, 4,000 simulated tasks with partial reads and two reads in one task, and 600 with interrupts and kills, on 16 and 12 threads, left nothing allocated or cached.Benchmark
A local benchmark that is not part of this PR, in the shape of #6806.
facthas key, long, double and string columns in one Parquet file per probe task, about 65K rows each.dimhas unique keys with a long and a string column. The query isSELECT /*+ BROADCAST(d) */ sum(f.f_long), sum(d.d_long), sum(length(f.f_str)), sum(length(d.d_str)) FROM fact f JOIN dim d ON f.key = d.k, with AQE off. Each figure is the median of 8 rounds that alternate the setting, after 2 warmup rounds. Run time and CPU are summed over the probe stage's tasks.local[8]local[8]local[8]local[1]Under
local[1]no two tasks overlap, so nothing is shared, and the difference is within run-to-run noise. One decode of the 1M-row broadcast (16.9 MB compressed, 31.8 MB of Arrow IPC), timed on the driver, took 16.4 ms throughChannels.newChanneland 16.0 ms through the new channel.Environment: Comet main at 66c63bd plus the first commit of this PR (afa7136), release build, Spark 4.1.3, Scala 2.13, JDK 17.0.10, macOS 26.6.2 on an Apple M3 Max.