perf(join): skip per-cell key null tests on provably null-free columns - #598
Merged
Merged
Conversation
ray_vec_is_null is out-of-line (no LTO) and the join called it once per key column per row in hash_row_keys — on the build side, the probe side, and the prefetch lookahead — plus twice per key column per hash-chain step in join_keys_eq, across both the count and the fill pass. A reported profile put 13.28% of a service's samples there, on a join keyed by two SYM columns that structurally never hold a null. Prove once per join that no key column can hold a null and drop the call. SYM/STR nulls are canonical empty payloads (id 0 / length 0) that HAS_NULLS does not track, so text columns are proven by the chunked zero-scan from #533 rather than by the flag; everything else reads the flag through slices and takes a set bit at face value, so the proof stays O(n) and never degrades into ray_vec_has_nulls' per-element walk. Flag-readable columns are settled first, so a nullable numeric key short-circuits before any text column is scanned. OP_CONST (atom) key slots are refused as unprovable. Also skip the #458 null-run pre-scan under the proof. It gates on ray_vec_may_have_nulls, which is unconditionally true for SYM/STR, so a SYM-keyed join ran a full per-key-per-build-row ray_vec_is_null scan before the join proper on every execution; a null-free key set cannot contain an all-null row, so the scan is dead. Measured with bench/join_nullfree (release, 1:1 book join, 4M probe x 500K build): two SYM keys 275 -> 249 ms (-9.5% median, -7.8% min); single I64 key 106 -> 99 ms (-6.3% median, -7.9% min). A proof that fails on a text key costs a partial scan (~8ms on a 4M-row SYM column) and gains nothing — only nullable SYM/STR key columns pay it. ray_join_force_null_checks forces the null-aware loops so the differential tests and the perf gate can compare both paths in one binary; ray_join_nullfree_keys counts the joins that took the fast path. Closes #597
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.
What & why
Fixes #597.
ray_vec_is_nullis out-of-line (no LTO) and the join called it once per key column per row inhash_row_keys— on the build side, the probe side, and the prefetch lookahead — plus twice per key column per hash-chain step injoin_keys_eq, across both the count and the fill pass. The reporter profiled 13.28% of a service's samples there, on a join keyed by two SYM columns that structurally never hold a null.This proves once per join that no key column can hold a null, and drops the call.
The flag-based fix the issue asked for would not have worked for the reported case.
ray_vec_is_nullreturns for SYM and STR before it readsHAS_NULLS(vec.c:1601-1605): text nulls are canonical empty payloads (id 0 / length 0) and the flag is deliberately not trusted there, which is also whyray_vec_may_have_nullsreturnstrueunconditionally for SYM/STR. So text columns are proven by the chunked zero-scan from #533 instead. Everything else reads the flag through slices and takes a set bit at face value, keeping the proof O(n) rather than degrading intoray_vec_has_nulls' per-element walk. Flag-readable columns are settled first so a nullable numeric key short-circuits before any text column is scanned;OP_CONST(atom) key slots are refused as unprovable (a guard — nothing currently builds such a key).The proof is threaded into
hash_row_keys,join_keys_eq, and all four worker contexts plus the anti-join driver.Second find
The #458 null-run pre-scan gates on
ray_vec_may_have_nulls, which is unconditionally true for SYM/STR — so a SYM-keyed join ran a full per-key-per-build-rowray_vec_is_nullscan before the join proper, on every execution. A null-free key set cannot contain an all-null row, so that scan is now skipped. This is plausibly the larger half of the reported 13.28%.Measurements
bench/join_nullfree, release build, 8 cores, 1:1 book join (4M probe x 500K build, 4M rows out), 11 reps interleaved, baseline = forced null-aware loops in the same binary:The control is the honest cost: a proof that fails on a text key scans part of the payload (~8 ms on a 4M-row SYM column) and gains nothing. Only nullable SYM/STR key columns pay it; numeric keys settle on an O(1) flag read.
Note for anyone re-running this: a correlated key generator collapses the join into a huge fan-out where gather dominates and the result is diluted and misleading. The benchmark uses unique right keys for that reason.
Not included
The issue's fallback ask, an explicit
.attr.set 'not-nulla caller can assert, is deliberately not implemented. A user-assertable not-null flag turns a slow join into a silently wrong one whenever the assertion is mistaken, and the payload scan costs only ~8 ms. #595 (a join site in the index routing table) is the better home for skipping the scan entirely.Test knobs
ray_join_force_null_checksforces the null-aware loops, so the differential tests and the perf gate compare both paths in one binary.ray_join_nullfree_keyscounts joins that took the fast path.Checklist
dev(notmaster)makebuilds cleanly (no new warnings; clean under-Werrorin debug and-O3release)make testpasses; tests added — 3920/3920 pass, 0 UBSan runtime errorsNew tests in
test/test_join_buildside.c, each watched failing first:nf_sym_keys_prove— null-free SYM keys take the fast pathnf_sym_null_blocks— one null SYM cell blocks it, and asserts the column carries noHAS_NULLS, pinning the reason the flag alone is insufficientnf_i64_flag_decides— the flag decides for numerics, both waysnf_differential— inner/left/full x chained and radix sizes x nullable and null-free, against the forced null-aware oracle