kumarUjjawal commented on code in PR #26956:
URL: https://github.com/apache/datafusion/pull/26956#discussion_r4430217850
##########
datafusion/common/benches/float_normalization.rs:
##########
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: Apache-2.0
+// SPDX-FileCopyrightText: See the NOTICE file distributed with this work for
additional information regarding copyright ownership
+// SPDX-FileContributor: Licensed to the Apache Software Foundation (ASF)
under one or more contributor license agreements
+
+//! Benchmarks for `normalize_floats`, which runs on every float key
+//! column before it is compared, hashed or grouped. The common case is a
+//! batch with nothing to normalize, where only the check runs.
+
+use std::hint::black_box;
+use std::sync::Arc;
+
+use arrow::array::{ArrayRef, Float32Array, Float64Array};
+use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main};
+use datafusion_common::float_semantics::normalize_floats;
+use rand::rngs::StdRng;
+use rand::{Rng, SeedableRng};
+
+const BATCH_SIZE: usize = 8192;
+
+fn values(rng: &mut StdRng) -> Vec<f64> {
+ (0..BATCH_SIZE)
+ .map(|_| rng.random_range(-1000.0..1000.0))
+ .collect()
+}
+
+fn bench_float_normalization(c: &mut Criterion) {
+ let mut group = c.benchmark_group("float_normalization");
+ let mut rng = StdRng::seed_from_u64(42);
+ let clean = values(&mut rng);
+ // A single `-0.0` in the last row: the check scans everything, then
copies.
+ let mut dirty = clean.clone();
+ dirty[BATCH_SIZE - 1] = -0.0;
+
+ for (name, values) in [("clean", &clean), ("neg_zero_last", &dirty)] {
Review Comment:
The benchmark only covers the cases this change speeds up. It has no case
with -0.0 in the first row, which is the one case this change makes slower.
##########
datafusion/common/src/float_semantics.rs:
##########
@@ -159,7 +159,13 @@ where
T::Native: FloatSemantics,
{
let arr = array.as_primitive::<T>();
- if !arr.values().iter().any(|v| v.needs_normalization()) {
+ // Not `any()`: its early exit checks one value at a time, while a fold
+ // over every value vectorizes.
+ let needs_normalization = arr
Review Comment:
The fold now always reads the whole column, even when a -0.0 is in the first
row. Folding in fixed chunks (e.g. 512 values) and stopping after the first
chunk that finds one would keep the SIMD speedup and still exit early.
##########
datafusion/common/benches/float_normalization.rs:
##########
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: Apache-2.0
+// SPDX-FileCopyrightText: See the NOTICE file distributed with this work for
additional information regarding copyright ownership
+// SPDX-FileContributor: Licensed to the Apache Software Foundation (ASF)
under one or more contributor license agreements
+
+//! Benchmarks for `normalize_floats`, which runs on every float key
+//! column before it is compared, hashed or grouped. The common case is a
+//! batch with nothing to normalize, where only the check runs.
+
+use std::hint::black_box;
+use std::sync::Arc;
+
+use arrow::array::{ArrayRef, Float32Array, Float64Array};
+use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main};
+use datafusion_common::float_semantics::normalize_floats;
+use rand::rngs::StdRng;
+use rand::{Rng, SeedableRng};
+
+const BATCH_SIZE: usize = 8192;
+
+fn values(rng: &mut StdRng) -> Vec<f64> {
+ (0..BATCH_SIZE)
+ .map(|_| rng.random_range(-1000.0..1000.0))
+ .collect()
+}
+
+fn bench_float_normalization(c: &mut Criterion) {
+ let mut group = c.benchmark_group("float_normalization");
+ let mut rng = StdRng::seed_from_u64(42);
+ let clean = values(&mut rng);
+ // A single `-0.0` in the last row: the check scans everything, then
copies.
+ let mut dirty = clean.clone();
+ dirty[BATCH_SIZE - 1] = -0.0;
+
+ for (name, values) in [("clean", &clean), ("neg_zero_last", &dirty)] {
+ let f64_array: ArrayRef = Arc::new(Float64Array::from(values.clone()));
+ group.bench_with_input(BenchmarkId::new("f64", name), &f64_array, |b,
a| {
+ b.iter(|| black_box(normalize_floats(black_box(a))))
+ });
+
+ let f32_values: Vec<f32> = values.iter().map(|&v| v as f32).collect();
Review Comment:
The benchmark measures f32 and f64 but not Float16, which goes through the
same changed fold.
##########
datafusion/common/benches/float_normalization.rs:
##########
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: Apache-2.0
+// SPDX-FileCopyrightText: See the NOTICE file distributed with this work for
additional information regarding copyright ownership
+// SPDX-FileContributor: Licensed to the Apache Software Foundation (ASF)
under one or more contributor license agreements
+
+//! Benchmarks for `normalize_floats`, which runs on every float key
+//! column before it is compared, hashed or grouped. The common case is a
+//! batch with nothing to normalize, where only the check runs.
+
+use std::hint::black_box;
+use std::sync::Arc;
+
+use arrow::array::{ArrayRef, Float32Array, Float64Array};
+use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main};
+use datafusion_common::float_semantics::normalize_floats;
+use rand::rngs::StdRng;
+use rand::{Rng, SeedableRng};
+
+const BATCH_SIZE: usize = 8192;
+
+fn values(rng: &mut StdRng) -> Vec<f64> {
+ (0..BATCH_SIZE)
+ .map(|_| rng.random_range(-1000.0..1000.0))
+ .collect()
+}
+
+fn bench_float_normalization(c: &mut Criterion) {
+ let mut group = c.benchmark_group("float_normalization");
+ let mut rng = StdRng::seed_from_u64(42);
+ let clean = values(&mut rng);
+ // A single `-0.0` in the last row: the check scans everything, then
copies.
+ let mut dirty = clean.clone();
+ dirty[BATCH_SIZE - 1] = -0.0;
+
+ for (name, values) in [("clean", &clean), ("neg_zero_last", &dirty)] {
Review Comment:
Benchmark negative zero near the start of the batch
--
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]
