Vivek1106-04 commented on PR #57528: URL: https://github.com/apache/spark/pull/57528#issuecomment-5187811257
The consecutive layout was the whole reason that bound looked comfortable: 448 bytes retained for the status, because 24,999 consecutive ids collapse into one run container. Interleaving the same size distribution retains 8,632 bytes, so the old `retained < 8 * 1024` assertion would indeed have rejected it. Both layouts are covered now, and the bound is stated in terms of the id space instead of a constant: a RoaringBitmap over ids below 65,536 holds one container, and a container denser than 4,096 ids becomes a fixed 8 KiB bitmap, so no layout of these ids can cost more than that. The serialized size is asserted too (8,372 bytes interleaved). On bounding the tie metadata independently of the reducer count, I don't think that is reachable together with what you asked for on Aug 3, and I'd rather say so than pretend otherwise. What the bitmap costs is a bit per reduce partition. That is the same bound `emptyBlocks` in this class has always had, and I measured it on the same layouts you did: | layout | tie bitmap | `emptyBlocks` on the identical id layout | |---|---|---| | 50,000 reducers, consecutive | 18 retained / 15 serialized | 18 / 15 | | 50,000 reducers, alternating | 8,202 / 8,208 | 8,202 / 8,208 | | 1,048,576 reducers, alternating | 131,112 / 131,208 | 131,112 / 131,208 | At the whole-status level the interleaved tie status retains 8,632 bytes, against 8,656 bytes for a status holding those very ids as empty blocks. Recording the ties costs slightly less than the bitmap this class already carried, and the new test asserts that relationship directly, so it holds across RoaringBitmap upgrades rather than resting on today's container sizes. The ties also cannot be more than half the blocks. `recordSkewedTies` requires `skewThreshold <= skewCutoff`, and `skewThreshold` is `max(medianSize * accurateBlockSkewedFactor, skewCutoff)` when it is below `spark.shuffle.accurateBlockThreshold`, so it requires `medianSize * 5 <= skewCutoff`. The median block is therefore strictly smaller than the cutoff, and fewer than half the blocks can sit at it. Your 24,999-of-50,000 construction is right at that ceiling. If the cutoff is at or above `spark.shuffle.accurateBlockThreshold` instead, the ties are recorded through the mandatory-accuracy path that predates this PR and never reach the bitmap. I added a comment recording both facts. That leaves the tension: capping tie cardinality is what makes the cost reducer-count-independent, and it is also exactly what made genuinely skewed reducers invisible in your Aug 3 reproduction, since every mapper sees the same distribution and the unrecorded ties fall back to `avgSize` and inflate it. Distinguishing an arbitrary ~n/2 subset of n reducers needs Θ(n) bits however it is encoded, so I don't see a representation that keeps AQE visibility and drops the per-reducer term. What is still bounded by `spark.shuffle.maxAccurateSkewedBlockNumber` is the id-and-size array, which was the unbounded cost in the original finding: at most 100 entries of 5 bytes, regardless of how many ties exist. For scale, on the two-population distribution from your own Aug 3 example (2,001 reducers, 200 tied), the bitmap is 414 bytes retained. The 8 KiB and 131 KiB figures need roughly half the reducers tied at the cutoff and scattered across every 64Ki block of the id space. An operator who does hit that and would rather not pay for it can set `spark.shuffle.accurateBlockSkewedFactor` to -1, which restores the previous behavior. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
