jayzhan211 commented on code in PR #26191:
URL: https://github.com/apache/datafusion/pull/26191#discussion_r4336667860
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -346,6 +340,82 @@ fn hash_array_primitive<T>(
}
}
+/// Overwrite each slot with `value.hash_one(state)`, 16 rows per iteration.
+/// Gathering + hashing a whole lane batch before storing keeps the independent
+/// hashes in flight so the pipeline isn't serialized on store-to-load deps.
+#[cfg(not(feature = "force_hash_collisions"))]
+#[inline(always)]
+fn hash_prim_fresh_dense<N, S>(values: &[N], hashes: &mut [u64], state: &S)
Review Comment:
nit: these loops can be written without any `unsafe` using `as_chunks`,
which gives fixed-size `[N; LANES]` chunks, so no bounds checks are left to
elide. It also avoids having safe helpers that rely on a `debug_assert` for
soundness. In my local runs it was at least as fast, but my machine was noisy,
so it's worth a `run benchmark with_hashes` to confirm:
```rs
fn hash_prim_fresh_dense<N, S>(values: &[N], hashes: &mut [u64], state: &S)
where
N: HashValue + Copy,
S: HashState,
{
const LANES: usize = 16;
let (value_chunks, value_tail) = values.as_chunks::<LANES>();
let (hash_chunks, hash_tail) = hashes.as_chunks_mut::<LANES>();
for (h, v) in hash_chunks.iter_mut().zip(value_chunks) {
*h = v.map(|value| value.hash_one(state));
}
for (h, v) in hash_tail.iter_mut().zip(value_tail) {
*h = v.hash_one(state);
}
}
fn hash_prim_rehash_dense<N, S>(values: &[N], hashes: &mut [u64], state: &S)
where
N: HashValue + Copy,
S: HashState,
{
const LANES: usize = 8;
let rehash_one = |prev_hash: u64, value: N| {
let mut hasher = state.seeded_state(prev_hash).build_hasher();
value.hash_write(&mut hasher);
hasher.finish()
};
let (value_chunks, value_tail) = values.as_chunks::<LANES>();
let (hash_chunks, hash_tail) = hashes.as_chunks_mut::<LANES>();
for (h, v) in hash_chunks.iter_mut().zip(value_chunks) {
*h = std::array::from_fn(|lane| rehash_one(h[lane], v[lane]));
}
for (h, &v) in hash_tail.iter_mut().zip(value_tail) {
*h = rehash_one(*h, v);
}
}
```
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -321,15 +321,9 @@ fn hash_array_primitive<T>(
if array.null_count() == 0 {
if rehash {
- for (hash, &value) in
hashes_buffer.iter_mut().zip(array.values().iter()) {
- let mut hasher =
random_state.seeded_state(*hash).build_hasher();
- value.hash_write(&mut hasher);
- *hash = hasher.finish();
- }
+ hash_prim_rehash_dense(array.values(), hashes_buffer,
random_state);
Review Comment:
nit: none of the `hash_utils` tests check the unrolled output. If I make
either loop store or read the wrong lane, all 45 module tests still pass. A
small test comparing each row with hashing a 1-row slice (which only goes
through the scalar tail) catches both:
```rs
#[test]
#[cfg(not(feature = "force_hash_collisions"))]
fn create_hashes_primitive_no_nulls_matches_per_row() -> Result<()> {
let random_state = RandomState::with_seed(0);
// lengths around the 8- and 16-row unrolled boundaries
for len in [0, 7, 8, 9, 15, 16, 17, 33] {
let a: ArrayRef = Arc::new(Int64Array::from_iter_values(0..len as
i64));
let b: ArrayRef =
Arc::new(Int32Array::from_iter_values((0..len as i32).rev()));
let mut hashes = vec![0; len];
create_hashes([&a, &b], &random_state, &mut hashes)?;
for (i, hash) in hashes.iter().enumerate() {
let mut expected = [0];
create_hashes(
[&a.slice(i, 1), &b.slice(i, 1)],
&random_state,
&mut expected,
)?;
assert_eq!(*hash, expected[0], "len {len}, row {i}");
}
}
Ok(())
}
```
--
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]
