alamb commented on code in PR #26159:
URL: https://github.com/apache/datafusion/pull/26159#discussion_r4234048791
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -538,28 +668,82 @@ fn hash_dictionary_scatter<
hashes_buffer: &mut [u64],
) {
let dict_values = array.values();
- if HAS_NULL_KEYS {
- for (hash, key) in hashes_buffer.iter_mut().zip(array.keys().iter()) {
- if let Some(key) = key {
- let idx = key.as_usize();
- if !HAS_NULL_VALUES || dict_values.is_valid(idx) {
Review Comment:
did you check how much faster we could get just by removing these bounds
checks?
For example
```rust
*hash =
combine_hashes(dict_hashes.get_unchecked(idx), *hash);
```
That might be enough to get LLVM to vectorize the code better /
automatically for you
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -538,28 +668,82 @@ fn hash_dictionary_scatter<
hashes_buffer: &mut [u64],
) {
let dict_values = array.values();
- if HAS_NULL_KEYS {
- for (hash, key) in hashes_buffer.iter_mut().zip(array.keys().iter()) {
- if let Some(key) = key {
- let idx = key.as_usize();
- if !HAS_NULL_VALUES || dict_values.is_valid(idx) {
Review Comment:
You could also potentially pull each branch into its own "inline_never"
function and allow the dispatch code (hash_dictionary_scatter) to be inlined
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -519,6 +517,138 @@ fn hash_generic_byte_view_array<T: ByteViewType>(
}
}
+/// Dense-path unroll width; 8 lanes keep the gather pipeline full on
ARM64/x86.
+#[cfg(not(feature = "force_hash_collisions"))]
+const DICT_SCATTER_LANES: usize = 8;
+
+/// Fold `dict_hash` into `prev` when `MULTI_COL`, else
+/// overwrite with `dict_hash`.
+#[cfg(not(feature = "force_hash_collisions"))]
+#[inline(always)]
+fn maybe_combine_dict_hash<const MULTI_COL: bool>(prev: u64, dict_hash: u64)
-> u64 {
+ if MULTI_COL {
+ combine_hashes(dict_hash, prev)
+ } else {
+ dict_hash
+ }
+}
+
+/// Scatter precomputed dict-value hashes into every key position with no
+/// null checks, 8-way unrolled.
+///
+/// The 8-wide unroll is for performance, splitting the loop into
Review Comment:
You might be able to get this to happen automatically by just removing the
bounds checks on the array access. That would be my preference rather than this
manual loop unrolling if possible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
