Repository navigation
Attach RowFn validity directly to canonical output - #9694
connortsui20 wants to merge 1 commit into
Conversation
Merging this PR will degrade performance by 17%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | arrow_checked_add_u32_avx2[16384] |
17.7 µs | 21.3 µs | -17% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ct/row-fn-canonical-validity (8208687) with ct/primitive-comparison-simd (6bb6d93)3
Footnotes
-
206 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
4 benchmarks were run, but are now archived. If they were deleted in another branch, consider rebasing to remove them from the report. Instead if they were added back, click here to restore them. ↩
-
No successful run was found on
ct/primitive-comparison-simd(d7f1683) during the generation of this report, so 4e4e412 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
84cde9e to
c72fbbe
Compare
f21a243 to
0945a6c
Compare
c72fbbe to
8df4c19
Compare
0945a6c to
740cd82
Compare
|
generally not a fan of optimisations like these... |
|
I think it shows a gap in our abstraction, ideally this is not required for this optimization? |
740cd82 to
ffca59e
Compare
8df4c19 to
b464dd5
Compare
b464dd5 to
f17be3d
Compare
f17be3d to
89e5590
Compare
| Validity::Array(validity_array) => { | ||
| let values = if let Some(canonical) = values.as_opt::<AnyCanonical>() { | ||
| mask_validity_canonical( | ||
| Canonical::from(canonical), | ||
| Validity::Array(validity_array), | ||
| ctx, | ||
| )? | ||
| .into_array() | ||
| } else { | ||
| values.mask(validity_array)? | ||
| }; | ||
|
|
||
| self.finalize_output(values, self.row_count) | ||
| } |
There was a problem hiding this comment.
There's something sketchy in the profile here and I would have to trace more of the code to figure out what's wrong. In the develop version we seem to execute the mask while in the new one we don't. Ah... I think this is because the benchmark is executing to AnyCanonical and this change here immediately flattens the mask into value instead of doing another execution round. Ultimately I think this is not useful benchmaxxing.
Signed-off-by: Connor Tsui <[email protected]>
89e5590 to
8208687
Compare
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
see above |
## Summary - Tracking Issue: #9128 - Supersedes #10674 - Related discussion: #9694 - Independent float32 candidate: #10741 Attaches input validity directly to canonical RowFn output using the existing helper. Preserves kernel-output validation, relabeling, outer nullability, and the non-canonical mask fallback. `values.mask(valid)` builds and optimizes a `ScalarFnArray` to dispatch through parent-reduce rules. The benchmark-driven bypass stays inside RowFn to avoid adding canonical-specific dispatch to the generic builtin. It can be removed if `.mask()` gains equivalent direct dispatch. This revisits the abstraction concern in #9694. **Draft pending fresh isolated benchmarks on this head.** Saved results include regressions and do not establish a general speedup. <details> <summary>Saved benchmark evidence and validation</summary> The isolated validity snapshot `b1961452ea3505d4e2588e34b14c20ea177ed175` was compared with frozen `develop` at `a1693389301e446eec41373cb274d98a303d3ca0`. Measurements used Rust 1.99.1 / LLVM 23.1.1, 16 CGUs, no LTO, CPU 2, and 21 balanced paired blocks on Graviton3 C7g and Sapphire Rapids C7i. This was native wall time, not CodSpeed simulation. For nullable u64 addition with 64 rows and a right-hand constant, median paired time ratios were: | Target | Candidate / baseline time | 95% paired bootstrap interval | | --- | ---: | ---: | | NEON | 0.798 | 0.796–0.802 | | AVX2 | 0.850 | 0.846–0.852 | | AVX-512 | 0.846 | 0.844–0.855 | The isolated snapshot also had flagged regressions outside that case: roughly 3.4% slower ARM u64 multiply and up to 4.2% slower AVX2 byte comparisons. No regressions crossed the 3% flag on AVX-512 in that run. These movements remain part of the review, even where unchanged code is sensitive to compiler context or binary layout. A separate combined packing/validity candidate at `8d8ef8151454bbc73e1cd0b236567542a1401cbe`, based on `d834a11faa79370b4d2e0bbcc73195f451a8a212`, retained the small-batch gain but regressed nullable u64 add by about 12% at 8,192 rows and 18% at 32,768 rows on AVX2. Profiling found a byte-identical retry hot loop with essentially unchanged instructions per operation and more cycles. The precise placement-sensitive mechanism remains unresolved. That combined result does not isolate this PR. The combined candidate passed 225 RowFn tests and 232 binary-function tests on that newer base, including output validation, allocator, and deferred-error coverage. Separate native probes checked null-row arithmetic errors and comparison edge cases. The changed file passed the pinned formatter. These are saved checks, not fresh validation of this individual PR. This PR is rebased onto `c750fbd80882f78efd6a2703bc0f3e17f1aca9f9`. The patch is unchanged by the rebase. Tests and benchmarks were not rerun after it. </details> --------- Signed-off-by: Connor Tsui <[email protected]>
Summary
Attaches input-derived validity directly when a RowFn kernel already returns a canonical array, avoiding an extra lazy
vortex.maskexecution. Non-canonical output keeps the existing lazy mask path.Changes
Reuses
mask_validity_canonicalwithout copying canonical value buffers and preserves kernel-output validation, output relabeling, and outer nullability. Structural Boolean and primitive tests verify that nullable dense execution no longer runsvortex.mask.