andygrove commented on code in PR #6430:
URL: https://github.com/apache/datafusion-comet/pull/6430#discussion_r4208443481
##########
native/shuffle/benches/shuffle_writer.rs:
##########
@@ -191,6 +194,72 @@ fn criterion_benchmark(c: &mut Criterion) {
/// Times one execution of a freshly built writer per iteration. A
`ShuffleWriterExec`
/// publishes its partition offsets once, so it cannot be re-executed;
building it is
/// setup and stays outside the measurement.
+// Include decimal SUM's non-null is_empty state, nullable Boolean-heavy rows,
+// and an Int64-only control. Both low and high partition counts matter because
+// short runs are more common with fewer hash partitions.
+fn boolean_state_benchmark(c: &mut Criterion) {
+ let mut group = c.benchmark_group("shuffle_writer_boolean_state");
+ group.sample_size(10);
+ for shape in ["decimal_sum", "nullable_booleans", "int64_control"] {
+ for partitions in [4, 200] {
+ for key_run in [1, 64] {
+ let schema_batch = boolean_state_batch(8192, shape, key_run);
+ group.bench_function(
+
format!("{shape}/partitions={partitions}/key_run={key_run}"),
+ |b| {
+ bench_end_to_end(b, || {
+ let batches = vec![schema_batch.clone(); 100];
Review Comment:
These 100 batches are clones of one batch, so they share every buffer and
every key. The gather keeps rereading one cache-resident batch, and each
partition's output is the same rows repeated 100 times. The base writer takes
21.9 ms on the clones against 63.2 ms on distinct batches, while the gather
alone only moves from 8.7 to 13.2 ms. So most of that gap is encoding and
compressing repeated rows, and it inflates the Boolean share. With the clones,
the decimal `SUM` state at 200 partitions went from 21.9 to 13.7 ms (-37%) in
my runs. Built from 100 distinct batches it went from 63.2 to 57.3 ms (-9%). At
4 partitions it was -27% against -13.5%. `partitioning_benchmark` in this file
builds each batch separately with `create_batch_from` for this reason. Could
`boolean_state_batch` take the batch index and offset the keys and values by
it, so the 100 batches are distinct?
##########
native/shuffle/benches/shuffle_writer.rs:
##########
@@ -191,6 +194,72 @@ fn criterion_benchmark(c: &mut Criterion) {
/// Times one execution of a freshly built writer per iteration. A
`ShuffleWriterExec`
/// publishes its partition offsets once, so it cannot be re-executed;
building it is
/// setup and stays outside the measurement.
+// Include decimal SUM's non-null is_empty state, nullable Boolean-heavy rows,
+// and an Int64-only control. Both low and high partition counts matter because
+// short runs are more common with fewer hash partitions.
+fn boolean_state_benchmark(c: &mut Criterion) {
Review Comment:
This went in between `bench_end_to_end`'s `///` doc comment and the
function. So rustdoc now attaches "Times one execution of a freshly built
writer per iteration" to `boolean_state_benchmark`, and `bench_end_to_end` has
no doc. Could you move the two new functions above that doc block?
--
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]
