Skip to content

fix: benchmark black box handling #5558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
0ax1 merged 2 commits into from
Nov 27, 2025
Merged

fix: benchmark black box handling #5558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66b4b51
fix: black box input handling
0ax1 Nov 27, 2025
a96e962
hoist
0ax1 Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions vortex-array/benches/scalar_subtract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ fn scalar_subtract(bencher: Bencher) {
let chunked = ChunkedArray::from_iter([data1, data2]).into_array();

bencher.with_inputs(|| &chunked).bench_refs(|chunked| {
let array = vortex_array::compute::sub_scalar(*chunked, to_subtract.into()).unwrap();
divan::black_box(array);
vortex_array::compute::sub_scalar(*chunked, to_subtract.into()).unwrap()
Copy link
Contributor

@joseph-isaacs joseph-isaacs Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude

Copy link
Contributor

@claude claude bot Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude finished @joseph-isaacs's task —— View job

Code Review: Benchmark Black Box Handling

I've reviewed the changes to benchmark black box handling across the 5 modified files. Here's my analysis:

Summary of Changes

This PR systematically removes explicit divan::black_box() calls from benchmark outputs and instead relies on Divan's implicit black-boxing behavior through bench_refs() and bench_values(). The key pattern is:

Before:

bencher.bench_refs(|x| {
    let result = expensive_operation(x);
    divan::black_box(result);
});

After:

bencher.bench_refs(|x| expensive_operation(x));

Analysis

✅ Correct Approach

The changes are generally correct. According to Divan's documentation, bench_refs() and bench_values() already black-box their return values automatically. The explicit black_box() calls on outputs were redundant.

✅ Input Handling Improvement

The more significant improvement is moving loop parameters into with_inputs():

// Before - length captured from outer scope, harder to optimize
.with_inputs(|| buffer)
.bench_refs(|buffer| {
    for idx in 0..length { ... }  // length from closure capture
})

// After - length as explicit input, properly black-boxed
.with_inputs(|| (buffer, length))
.bench_refs(|(buffer, length)| {
    for idx in 0..*length { ... }  // length dereferenced from tuple
})

This ensures loop bounds are treated as benchmark inputs and prevents the compiler from constant-folding based on knowing length at compile time.

Issues & Concerns

⚠️ Performance Regression Mystery

The CodSpeed report shows 11 regressions despite 31 improvements. The most notable regression is in from_iter[Arrow<BooleanBuffer>, 128] (-31.48%). This is puzzling because:

  1. The from_iter benchmark itself wasn't modified in the diff
  2. The regression might indicate measurement instability or compiler optimization differences
  3. No explicit black-boxing was removed from from_iter - it never had any

🔍 Specific File Review

vortex-array/benches/scalar_subtract.rs (Line 38):

bencher.with_inputs(|| &chunked).bench_refs(|chunked| {
    vortex_array::compute::sub_scalar(*chunked, to_subtract.into()).unwrap()
});

✅ Correct - The result is now implicitly black-boxed by bench_refs()

vortex-buffer/benches/vortex_bitbuffer.rs (slice_arrow_buffer):
The changes to slice_arrow_buffer at lines 147-154 are particularly relevant given the PR title mentions this benchmark:

// After
bencher
    .with_inputs(|| (&buffer, length / 2))
    .bench_refs(|(buffer, mid)| {
        let mid = *mid;
        buffer.0.slice(mid / 2, mid / 2)
    });

✅ Much better - Now mid is properly an input parameter rather than being captured from the outer scope. This should make measurements more stable.

Recommendations

  1. Monitor the regressions: The from_iter regression seems unrelated to the changes and might indicate benchmark instability. Consider running benchmarks multiple times to verify stability.

  2. Consider one more optimization: In several places, you're still capturing length from the outer scope for buffer creation:

    let buffer = BitBuffer::from_iter((0..length).map(|i| i % 2 == 0));
bencher.with_inputs(|| (&buffer, length / 2))

    This is fine since the buffer creation happens outside the benchmark timing.

  3. Documentation: Consider adding a comment explaining Divan's implicit black-boxing behavior so future contributors don't re-add explicit black_box() calls.

Verdict

LGTM with monitoring recommended ✅

The changes correctly remove redundant black-boxing and improve input handling. The improvements in slice_arrow_buffer (the PR's focus) look good with +18.05% performance gains. The unrelated regressions warrant investigation but aren't necessarily caused by these changes.

});
}
12 changes: 3 additions & 9 deletions vortex-array/benches/take_struct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,7 @@ fn take_struct_simple(bencher: Bencher) {

bencher
.with_inputs(|| (&struct_array, &indices_array))
.bench_refs(|(array, indices)| {
divan::black_box(take(array.as_ref(), indices.as_ref()).unwrap());
});
.bench_refs(|(array, indices)| take(array.as_ref(), indices.as_ref()).unwrap());
}

#[divan::bench(args = [8])]
Expand Down Expand Up @@ -81,9 +79,7 @@ fn take_struct_wide(bencher: Bencher, width: usize) {

bencher
.with_inputs(|| (&struct_array, &indices_array))
.bench_refs(|(array, indices)| {
divan::black_box(take(array.as_ref(), indices.as_ref()).unwrap());
});
.bench_refs(|(array, indices)| take(array.as_ref(), indices.as_ref()).unwrap());
}

#[divan::bench]
Expand Down Expand Up @@ -111,7 +107,5 @@ fn take_struct_sequential_indices(bencher: Bencher) {

bencher
.with_inputs(|| (&struct_array, &indices_array))
.bench_refs(|(array, indices)| {
divan::black_box(take(array.as_ref(), indices.as_ref()).unwrap());
});
.bench_refs(|(array, indices)| take(array.as_ref(), indices.as_ref()).unwrap());
}
106 changes: 46 additions & 60 deletions vortex-buffer/benches/vortex_bitbuffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,45 +37,43 @@ fn from_iter<B: FromIterator<bool>>(n: usize) {
#[divan::bench(args = INPUT_SIZE)]
fn append_vortex_buffer(bencher: Bencher, length: usize) {
bencher
.with_inputs(|| BitBufferMut::with_capacity(length))
.bench_refs(|buffer| {
for idx in 0..length {
buffer.append(divan::black_box(idx % 2 == 0));
.with_inputs(|| (BitBufferMut::with_capacity(length), length))
.bench_refs(|(buffer, length)| {
for idx in 0..*length {
buffer.append(idx % 2 == 0);
}
});
}

#[divan::bench(args = INPUT_SIZE)]
fn append_arrow_buffer(bencher: Bencher, length: usize) {
bencher
.with_inputs(|| Arrow(BooleanBufferBuilder::new(length)))
.bench_refs(|buffer| {
for idx in 0..length {
buffer.0.append(divan::black_box(idx % 2 == 0));
.with_inputs(|| (Arrow(BooleanBufferBuilder::new(length)), length))
.bench_refs(|(buffer, length)| {
for idx in 0..*length {
buffer.0.append(idx % 2 == 0);
}
});
}

#[divan::bench(args = INPUT_SIZE)]
fn append_n_vortex_buffer(bencher: Bencher, length: usize) {
bencher
.with_inputs(|| BitBufferMut::with_capacity(length))
.bench_refs(|buffer| {
.with_inputs(|| (BitBufferMut::with_capacity(length), length, true))
.bench_refs(|(buffer, length, boolean)| {
for _ in 0..100 {
buffer.append_n(divan::black_box(true), divan::black_box(length / 100));
buffer.append_n(*boolean, *length / 100);
}
});
}

#[divan::bench(args = INPUT_SIZE)]
fn append_n_arrow_buffer(bencher: Bencher, length: usize) {
bencher
.with_inputs(|| Arrow(BooleanBufferBuilder::new(length)))
.bench_refs(|buffer| {
.with_inputs(|| (Arrow(BooleanBufferBuilder::new(length)), length, true))
.bench_refs(|(buffer, length, boolean)| {
for _ in 0..100 {
buffer
.0
.append_n(divan::black_box(length / 100), divan::black_box(true));
buffer.0.append_n(*length / 100, *boolean);
}
});
}
Expand All @@ -90,7 +88,7 @@ fn append_buffer_vortex_buffer(bencher: Bencher, length: usize) {
})
.bench_refs(|(source, dest)| {
for _ in 0..100 {
dest.append_buffer(divan::black_box(source));
dest.append_buffer(source);
}
});
}
Expand All @@ -107,7 +105,7 @@ fn append_buffer_arrow_buffer(bencher: Bencher, length: usize) {
})
.bench_refs(|(source, dest)| {
for _ in 0..100 {
for value in divan::black_box(&source.0).iter() {
for value in source.0.iter() {
dest.0.append(value);
}
}
Expand Down Expand Up @@ -141,104 +139,92 @@ fn slice_vortex_buffer(bencher: Bencher, length: usize) {
.with_inputs(|| (&buffer, length / 2))
.bench_refs(|(buffer, mid)| {
let mid = *mid;
divan::black_box(buffer.slice(mid / 2..mid + mid / 2));
buffer.slice(mid / 2..mid + mid / 2)
});
}

#[divan::bench(args = INPUT_SIZE)]
fn slice_arrow_buffer(bencher: Bencher, length: usize) {
let buffer = Arrow(BooleanBuffer::from_iter((0..length).map(|i| i % 2 == 0)));
bencher.with_inputs(|| &buffer).bench_refs(|buffer| {
let mid = length / 2;
divan::black_box(buffer.0.slice(mid / 2, mid / 2));
});
bencher
.with_inputs(|| (&buffer, length / 2))
.bench_refs(|(buffer, mid)| {
let mid = *mid;
buffer.0.slice(mid / 2, mid / 2)
});
}

#[divan::bench(args = INPUT_SIZE)]
fn true_count_vortex_buffer(bencher: Bencher, length: usize) {
let buffer = BitBuffer::from_iter((0..length).map(|i| i % 2 == 0));
bencher.with_inputs(|| &buffer).bench_refs(|buffer| {
divan::black_box(buffer.true_count());
})
bencher
.with_inputs(|| &buffer)
.bench_refs(|buffer| buffer.true_count())
}

#[divan::bench(args = INPUT_SIZE)]
fn true_count_arrow_buffer(bencher: Bencher, length: usize) {
let buffer = Arrow(BooleanBuffer::from_iter((0..length).map(|i| i % 2 == 0)));
bencher.with_inputs(|| &buffer).bench_refs(|buffer| {
divan::black_box(buffer.0.count_set_bits());
});
bencher
.with_inputs(|| &buffer)
.bench_refs(|buffer| buffer.0.count_set_bits());
}

#[divan::bench(args = INPUT_SIZE)]
fn bitwise_and_vortex_buffer(bencher: Bencher, length: usize) {
let a = BitBuffer::from_iter((0..length).map(|i| i % 2 == 0));
let b = BitBuffer::from_iter((0..length).map(|i| i % 3 == 0));
bencher
.with_inputs(|| {
let a = BitBuffer::from_iter((0..length).map(|i| i % 2 == 0));
let b = BitBuffer::from_iter((0..length).map(|i| i % 3 == 0));
(a, b)
})
.bench_values(|(a, b)| {
divan::black_box(&a & &b);
});
.with_inputs(|| (&a, &b))
.bench_values(|(a, b)| a & b);
}

#[divan::bench(args = INPUT_SIZE)]
fn bitwise_and_arrow_buffer(bencher: Bencher, length: usize) {
let a = Arrow(BooleanBuffer::from_iter((0..length).map(|i| i % 2 == 0)));
let b = Arrow(BooleanBuffer::from_iter((0..length).map(|i| i % 3 == 0)));
bencher.with_inputs(|| (&a, &b)).bench_refs(|(a, b)| {
divan::black_box(&a.0 & &b.0);
});
bencher
.with_inputs(|| (&a, &b))
.bench_refs(|(a, b)| &a.0 & &b.0);
}

#[divan::bench(args = INPUT_SIZE)]
fn bitwise_or_vortex_buffer(bencher: Bencher, length: usize) {
let a = BitBuffer::from_iter((0..length).map(|i| i % 2 == 0));
let b = BitBuffer::from_iter((0..length).map(|i| i % 3 == 0));
bencher
.with_inputs(|| {
let a = BitBuffer::from_iter((0..length).map(|i| i % 2 == 0));
let b = BitBuffer::from_iter((0..length).map(|i| i % 3 == 0));
(a, b)
})
.bench_values(|(a, b)| {
divan::black_box(&a | &b);
});
.with_inputs(|| (&a, &b))
.bench_values(|(a, b)| a | b);
}

#[divan::bench(args = INPUT_SIZE)]
fn bitwise_or_arrow_buffer(bencher: Bencher, length: usize) {
let a = Arrow(BooleanBuffer::from_iter((0..length).map(|i| i % 2 == 0)));
let b = Arrow(BooleanBuffer::from_iter((0..length).map(|i| i % 3 == 0)));
bencher.with_inputs(|| (&a, &b)).bench_refs(|(a, b)| {
divan::black_box(&a.0 | &b.0);
});
bencher
.with_inputs(|| (&a, &b))
.bench_refs(|(a, b)| &a.0 | &b.0);
}

#[divan::bench(args = INPUT_SIZE)]
fn bitwise_not_vortex_buffer(bencher: Bencher, length: usize) {
bencher
.with_inputs(|| BitBuffer::from_iter((0..length).map(|i| i % 2 == 0)))
.bench_values(|buffer| {
divan::black_box(!&buffer);
});
.bench_values(|buffer| !&buffer);
}

#[divan::bench(args = INPUT_SIZE)]
fn bitwise_not_vortex_buffer_mut(bencher: Bencher, length: usize) {
bencher
.with_inputs(|| BitBufferMut::from_iter((0..length).map(|i| i % 2 == 0)))
.bench_values(|buffer| {
divan::black_box(!buffer);
});
.bench_values(|buffer| !buffer);
}

#[divan::bench(args = INPUT_SIZE)]
fn bitwise_not_arrow_buffer(bencher: Bencher, length: usize) {
bencher
.with_inputs(|| Arrow(BooleanBuffer::from_iter((0..length).map(|i| i % 2 == 0))))
.bench_values(|buffer| {
divan::black_box(!&buffer.0);
});
.bench_values(|buffer| !&buffer.0);
}

#[divan::bench(args = INPUT_SIZE)]
Expand Down
11 changes: 3 additions & 8 deletions vortex-buffer/benches/vortex_buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,15 +123,10 @@ fn push_arrow_buffer(bencher: Bencher, length: i32) {
#[divan::bench(types = [u8, u16, u32, u64], args = INPUT_SIZE_USIZE)]
fn push_n_vortex_buffer<T: PrimInt>(bencher: Bencher, length: usize) {
bencher
.with_inputs(|| BufferMut::<T>::with_capacity(length))
.bench_refs(|buffer| {
.with_inputs(|| (BufferMut::<T>::with_capacity(length), length, T::one()))
.bench_refs(|(buffer, length, one)| {
for _ in 0..100 {
unsafe {
buffer.push_n_unchecked(
divan::black_box(T::one()),
divan::black_box(length / 100),
)
};
unsafe { buffer.push_n_unchecked(*one, *length / 100) };
}
});
}
Expand Down
5 changes: 1 addition & 4 deletions vortex-compute/benches/expand_buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,5 @@ fn expand_buffer<T: Copy + Default + From<u8> + Send + 'static>(
let buffer = create_test_buffer::<T>(true_count);
(buffer, mask)
})
.bench_refs(|(buffer, mask)| {
let result = buffer.expand(mask);
divan::black_box(result);
});
.bench_refs(|(buffer, mask)| buffer.expand(mask));
}
Loading