Repository navigation
Dispatch zip kernels on either value branch - #11138
Conversation
Normalize if_false dispatch by filling null mask entries with false before inversion. This preserves SQL ELSE semantics while allowing both execute and reduce kernels to handle either branch. Refs #10908. Signed-off-by: blaginin <[email protected]>
Merging this PR will improve performance by 18.33%2 warnings
Performance Changes
Tip Curious why performance improved? Comment Comparing Footnotes
|
connortsui20
left a comment
There was a problem hiding this comment.
I wonder if there is a more general way to do this so that it is harder to get this wrong? I mostly relied on an LLM to write this so I didn't even consider matching directly on the parent to get the siblings. Should kernels have a standard way to get siblings? cc @robert3005 @joseph-isaacs
|
I think this is very much function dependent if you can do this kind of switch. I think there's some prior art in languages like objective-c how to do it but I am not too familiar |
|
Maybe just need to add some docs instead? |
## Summary Follow-up to #11110. Keep the remaining noisy cases out of automatic CodSpeed runs while preserving them for local benchmarks. The Python scalar exclusions in #11110 never took effect because the runner exports simulation as `instrumentation`, while the fixture checked for `simulation`. Use the plugin's resolved mode so these cases are actually skipped. ## Changes - Allow a trailing comma in `#[divan::bench(...)]` when the CPU-feature macro appends its options. The multiline bitpacked take definition covers this case in CI builds. - Share the resolved simulation check between the scalar and filter fixtures. Also skip the 2 integer one-shot filtered scans that still report system calls under simulation. - Keep AVX2 bitpacked take with 80 selections local after batching still produced a regression alert. Retain the 64-selection case and both AVX-512 cases in CI. - Keep AVX-512 nullable integer comparison local because its minimum changes by more than 10% while its median stays stable. - Keep the small random power-law boolean filter and the large correlated power-law boolean filter local under simulation. The former has repeated unexplained changes, and the latter repeatedly makes system calls that simulation cannot measure. <details> <summary>Evidence from recent PR reports</summary> Reviewed 40 CodSpeed report comments updated between November 2 at 19:28 UTC and November 3 at 19:28 UTC, including their comparison revisions and the affected measurement modes. | Case | Evidence | | --- | --- | | Python integer scalar scans | [#11154](#11154 (comment)) still reports the repeated scan's 10 system calls. [A develop run after #11110](https://github.com/vortex-data/vortex/actions/runs/41677647530/job/126062463160) executes both integer scalar cases instead of skipping them. The [runner exports the legacy `instrumentation` value](https://github.com/CodSpeedHQ/codspeed/blob/main/crates/runner-shared/src/measurement_mode.rs), which pytest-codspeed 5.0.3 normalizes to simulation. | | Python integer filtered scans | [#11130](#11130 (comment)) reports 30 system calls totaling 14.3 ms in `test_scan_filter[int]`. [#11138](#11138 (comment)) reports 30 calls totaling 16.3 ms in `test_scan_filter_to_arrow_array[int]`. | | AVX2 `threshold[80]` | [#11004](#11004 (comment)) compares `f7b5b1d864` with `e59b236d7b`: 2.183 to 2.531 µs, reported as a 13.75% regression. Both use 1000 samples of 16 iterations. The head median is 2.556 µs, so this is not just a single outlying minimum. The PR does not change the fastlanes code or benchmark. | | AVX-512 `lt_i64_nullable` | [#11168](#11168 (comment)) reports 7.342 to 8.217 µs. Several unrelated CUDA PRs report the opposite direction, including [#10872](#10872 (comment)), at 8.208 to 7.348 µs. Their head medians remain around 8.3 to 8.5 µs. | | Boolean `filter_powerlaw_by_random[10000]` | [#11167](#11167 (comment)) reports a 16.2% regression, [#11168](#11168 (comment)) a 23.56% improvement, and [#11169](#11169 (comment)) a 13.06% regression. The cause is unknown. | | Boolean `filter_powerlaw_by_correlated_runs[100000]` | [#10871](#10871 (comment)) and [#11165](#11165 (comment)) each report 2 system calls totaling about 44 µs. | The NEON chunked-take alerts on #11090 are on a PR that changes execution substantially, so those cases remain enabled. Other dictionary simulation alerts need more attribution before excluding them. </details> CI passes formatting, Python lint and script tests, Rust lint, default-feature and all-feature Rust builds, all 3 CPU walltime benchmark jobs, and Python benchmarks (36 passed, 4 intentionally skipped). The previously failed simulation shard also passes. Some remaining CI jobs and CodSpeed performance analysis are still pending. No local checks were run. The pre-commit and pre-push hooks were bypassed as requested. --------- Signed-off-by: Connor Tsui <[email protected]>
Normalize if_false dispatch by filling null mask entries with false before inversion.
Closes #10908.