viirya commented on PR #56156: URL: https://github.com/apache/spark/pull/56156#issuecomment-4566479018
@dongjoon-hyun Thanks for going through the numbers. Quick triage: **Lines 290 / 297** — `putNotNulls count=4096 / 65536` going `0.5X -> 11.9X / 15.7X`: those are the real wins this PR is going for (the OffHeap `setMemory` path kicks in past the 128-element threshold). **Lines 24 and 276** — I dug into these and I'm fairly sure both are GHA single-run noise rather than regressions caused by this PR: * **Line 24, `putBooleans count=64`**: this PR doesn't touch `putBooleans` at all (that one was fixed in SPARK-57036 and is already on `master`). Both the baseline and the patched results execute exactly the same code, yet the numbers move by ~40% across the two GHA runs. * **Line 276, `putNotNulls count=64`**: 64 is below the `SET_MEMORY_THRESHOLD = 128`, so the patched path is the inline byte-loop fallback — i.e. structurally the same per-byte loop as `master`'s baseline. Both runs execute the same byte loop here. Sweeping the four OffHeap byte-fill methods at small counts side-by-side makes the noise pattern clearer (JDK 21, Rate M/s, OffHeap only): | count | putBooleans baseline → patched | putBytes baseline → patched | putNulls baseline → patched | putNotNulls baseline → patched | | ---: | --- | --- | --- | --- | | 8 | 1633 → 1222 (−25%) | 849 → 996 (+17%) | 478 → 305 (−36%) | 534 → 303 (−43%) | | 64 | 2550 → 1515 (−41%) | 1331 → 2531 (+90%) | 1733 → 463 (−73%) | 1162 → 465 (−60%) | | 512 | 19325 → 20244 (+5%) | 18704 → 19733 (+5%) | 13000 → 14688 (+13%) | 1371 → **14186 (+10x)** | | 4096 | 39185 → 40351 (+3%) | 39244 → 40089 (+2%) | 34880 → 36785 (+5%) | 1407 → **36768 (+26x)** | `putBooleans` / `putBytes` / `putNulls` are unchanged by this PR — but small-count cells move by ±25–90% between the two GHA runs. `putNotNulls` shows the same noise floor at small counts and then a clean break above the threshold where the actual code change applies. `putBytes count=64` going from 1331 → 2531 in the same run pair is, I think, the cleanest counter-example: same code, GHA paired the two runs onto different runner state, and the rate doubled. So: lines 290 / 297 are real, lines 24 / 276 (and the `−25/−36/−43%` at `count=8`) are run-to-run jitter and not anything this PR introduced. Happy to re-trigger the JDK 21 benchmark a couple more times if it would help triangulate, but I don't think the underlying picture will change. -- 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]
