Repository navigation
perf: reuse cached dictionary value slots in vectorized_equal_to - #25652
Conversation
- Add `lookup_inner_slot`, resolving one `val_idx` against `value_dedup` and memoising the result in `val_to_inner`. - Call `sync_value_cache` from `vectorized_equal_to`. - Drop `build_lookup_table`, the `rhs_rows.len() < num_distinct` guard and `equal_to_per_row`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25652 +/- ##
==========================================
+ Coverage 82.49% 82.55% +0.05%
==========================================
Files 1140 1141 +1
Lines 438555 440284 +1729
Branches 438555 440284 +1729
==========================================
+ Hits 361803 363469 +1666
+ Misses 54880 54839 -41
- Partials 21872 21976 +104 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @jayzhan211 , could you PTAL when you have chances? Thanks! 🙏 |
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @CuteChuanChuan , here are some suggestions
| // so overwriting an already-false bit is a no-op. | ||
| let raw_keys = dict_keys.values(); | ||
| for (idx, (&lhs_row, &rhs_row)) in | ||
| lhs_rows.iter().zip(rhs_rows.iter()).enumerate() |
There was a problem hiding this comment.
Calling lookup_inner_slot(&mut self, ..) on every row makes the hot loop slower than the old lookup[..] slice index. The existing dictionary_group_values bench regresses: dict_intern_emit 1.21–1.71x at card ≤ 1000, dict_repeated_intern_emit 1.08–1.52x. In intern_emit the values Arc is already cached by vectorized_append, so this is the loop itself, not the removed fallback. Reading the cache inline and taking the resolver only on a miss brought intern_emit back to 0.96–1.05x in my run:
- let val_idx = raw_keys[rhs_row].as_usize();
- let rhs_slot = self.lookup_inner_slot(dict_values, val_idx);
+ let val_idx = raw_keys[rhs_row].as_usize();
+ let rhs_slot = match self.val_to_inner[val_idx] {
+ usize::MAX => self.lookup_inner_slot(dict_values, val_idx),
+ slot => slot,
+ };+ #[cold]
+ #[inline(never)]
fn lookup_inner_slot(&mut self, dict_values: &ArrayRef, val_idx: usize) -> usize {Apply the same change to the nullable-key loop and keep its get_bit guard. A merged single loop still left repeated_intern_emit (10% null keys) at 1.06–1.19x.
There was a problem hiding this comment.
Thanks for the numbers. I applied this and then ran the whole dictionary_group_values bench against main.
| let lookup = self.build_lookup_table(dict_values, &val_hashes); | ||
|
|
||
| let group_to_inner = self.group_to_inner.as_slice(); | ||
| self.sync_value_cache(dict_values); |
There was a problem hiding this comment.
Dropping the rhs_rows.len() < num_distinct fallback regresses the steady-state case with a new values array per batch: every row hits an existing group, so vectorized_append isn't called for the column. sync_value_cache now hashes all D values per batch where base did per-row equal_to. That happens with concatenated/coalesced dictionaries and at parquet row-group boundaries. With 8192 rows/batch and D = 100000, I measured 1.19–1.48x vs base. Keep the fallback, but only when the Arc isn't already cached (0.89–0.95x in my run):
let cached = self
.cached_values
.as_ref()
.is_some_and(|c| Arc::ptr_eq(c, dict_values));
if !cached && rhs_rows.len() < dict_values.len() {
self.equal_to_per_row(lhs_rows, dict_values, dict, rhs_rows, equal_to_results);
return;
}
self.sync_value_cache(dict_values);Please also add before/after bench numbers to the PR description, since this is a perf PR.
There was a problem hiding this comment.
Done, using your condition. The fallback now only runs when the values Arc isn't cached and rhs_rows.len() < dict_values.len().
- Restore the per-row fallback for uncached values arrays with fewer rows than values - On a cache miss, compare against the group's slot and cache only equal results - Borrow val_to_inner / group_to_inner as slices outside the hot loops - Add tests for cache fill, null value vs null key, and the fallback path
|
Benchmark:
repeated_intern_emit (4 batches, 10% null keys)
shared_values_arc (32 batches of 8192 rows sharing one values array)
|
|
run benchmark dictionary_group_values |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing issue-25219/reuse-cache-dic (40bfa2b) to 6a792c6 (merge-base) diff Run configurationrun benchmark dictionary_group_valuesResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing issue-25219/reuse-cache-dic (40bfa2b) to 6a792c6 (merge-base) diff Run configurationrun benchmark dictionary_group_valuesCPU Details (lscpu)Details
Resource Usagedictionary_group_values — base (merge-base)
dictionary_group_values — branch
File an issue against this benchmark runner |
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @CuteChuanChuan !
|
Hi @jayzhan211, About commit 40bfa2b, I went through three steps:
|
Upstream apache#25652 added Arc::ptr_eq checks against cached_values, which this branch changed to Option<Weak<dyn Array>>. Use the existing is_cached helper instead.
…che#25652) ## Which issue does this PR close? - Closes apache#25219. ## Rationale for this change Follow-up to apache#25185. `vectorized_equal_to` was left untouched and still rebuilt the same `val_idx -> inner_slot` map from scratch on every batch. ## What changes are included in this PR? - Added `lookup_inner_slot`, which resolves a single `val_idx` to its slot in `inner` and memorizes the result in `val_to_inner`. - Changed `vectorized_equal_to` to call `sync_value_cache` and resolve slots lazily per row. - Removed `build_lookup_table`, the `rhs_rows.len() < num_distinct` guard and `equal_to_per_row`. ## What is the testing strategy for this PR? Covered by the existing `dictionary.rs` unit tests. ## Are there any user-facing changes? No. This is an internal performance change with no public API or behaviour change.
Which issue does this PR close?
Rationale for this change
Follow-up to #25185.
vectorized_equal_towas left untouched and still rebuilt the sameval_idx -> inner_slotmap from scratch on every batch.What changes are included in this PR?
Added
lookup_inner_slot, which resolves a singleval_idxto its slot ininnerand memorizes the result inval_to_inner.Changed
vectorized_equal_toto callsync_value_cacheand resolve slots lazily per row.Removed
build_lookup_table, therhs_rows.len() < num_distinctguard andequal_to_per_row.What is the testing strategy for this PR?
Covered by the existing
dictionary.rsunit tests.Are there any user-facing changes?
No. This is an internal performance change with no public API or behaviour change.