Repository navigation
perf: skip null keys when scattering nullable dictionary hashes - #26966
lakshaydevoo-arch wants to merge 8 commits into
Conversation
|
@Rich-T-kid see the nullable path optimizations are feasible or any more requirements from your side. |
|
@jayzhan211 asking for a review sir |
|
👋 sorry for the delay Ill try and take a look at this |
|
run benchmark with_hashes |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/26848-dictionary-hash-scatter (aa745d4) to 5db4b23 (merge-base) diff Run configurationrun benchmark with_hashes
env:
BENCH_FILTER: "dictionary_utf8_int32"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/26848-dictionary-hash-scatter (aa745d4) to 5db4b23 (merge-base) diff Run configurationrun benchmark with_hashes
env:
BENCH_FILTER: "dictionary_utf8_int32"CPU Details (lscpu)Details
Resource Usagewith_hashes — base (merge-base)
with_hashes — branch
File an issue against this benchmark runner |
Rich-T-kid
left a comment
There was a problem hiding this comment.
@lakshaydevoo-arch can you split benchmarks into a separate PR so we can use the bot to compare benchmarks
|
thanks for pointing that out i will move the benchmarks addition to seprate PR so bot can review. |
|
I've separated the additional benchmarks into a dedicated PR (#27008 ) and removed them from this optimization PR. This PR now focuses on the nullable dictionary hash scattering optimization and its regression tests. The benchmark PR can be merged first so the bot can compare the same cases against the baseline. |
|
@Rich-T-kid you may look at the new PR #27008 i made |
Important
Fork CI setup: For pull requests from a fork to
main, enable workflows in your fork's Actions tab before you push.See the fork CI setup guide.
Which issue does this PR close?
This covers the nullable-key path only. As discussed with author, I'll send the non-null path as a separate PR.
Rationale for this change
When
create_hasheshashes a dictionary array, it hashes each dictionary value once and then scatters those hashes out to the rows through the keys. If the keys have nulls, that scatter loop goes throughkeys().iter(). It builds anOptionfor every row and branches on it, so the cost depends on the total number of rows, not the number of valid keys.The
with_hashesbenchmarks show this clearly. 97% null keys cost about as much as 3%, even though there's roughly 30x less real work to do. At 50% nulls the branch can't be predicted, so that case was the slowest of all.What changes are included in this PR?
hash_dictionary_scatter, the loop now goes overNullBuffer::valid_indices()instead ofkeys().iter(). Only valid key slots are visited, so:combine_hashespath and the existing behaviour of stopping athashes_buffer.len()are all unchanged.unsafe. The existingget_uncheckedon the dictionary hashes is now only reached from valid key slots.with_hashesbenchmark densities: Int32 dictionaries with 50% and 97% null keys, single and multiple columns. 0% and 3% were already covered by the existingdictionary_utf8_int32cases. The existing seeded null mask still produces exactly the same output.I also tried handling validity in 64-row words, with a dense loop for fully-valid words, as the issue suggests. On these benchmarks it was within ±4–9% of the simpler version, in both directions, so I left it out to keep the change small. I also checked whether key width matters (Int8 / Int32 / UInt64) and couldn't find a consistent difference, so I didn't add key-width benchmarks.
What is the testing strategy for this PR?
Added
create_hashes_for_dict_arrays_with_null_keysinhash_utils.rs. It compares dictionary hashes with the hashes of the equivalentStringArrayacross:-1,values.len(),127) hidden behind null slots;Int32column first, so "unchanged" can be told apart from "zero");hashes_buffershorter than the array;The test passes against the old loop too, so behaviour is unchanged. It fails if the loop is broken or loses the truncation.
Checked locally:
cargo fmt --all -- --checkcargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warningscargo test -p datafusion-common, both with and withoutforce_hash_collisionsBenchmarks
cargo bench -p datafusion-common --bench with_hashes -- dictionary, 3 runs on each side, median. Intel i5-12500H (WSL2). From what I measured, anything under about 6% is run-to-run noise on this machine.A few notes:
main's multiple / 50% number was oddly slow, about 2x the same case with Int8 keys, and I couldn't work out why. So part of that -85% is that slowdown going away, not just this change.Are there any user-facing changes?
No. This is an internal performance change with no API changes.