perf(ipc): benchmark stream encoder - #10447
Conversation
StreamEncoder benchmarks catch regressions without measuring sink IO. Closes apache#10373 Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
|
cc @Rich-T-kid |
Rich-T-kid
left a comment
Rich-T-kid
left a comment
There was a problem hiding this comment.
Thank you @Phoenix500526, I left a comment but overall this looks good to me
| let mut encoder = | ||
| StreamEncoder::try_new_with_options(batch.schema().as_ref(), options).unwrap(); | ||
| let mut encoded_len = 0; | ||
| for _ in 0..10 { | ||
| encoded_len += encoded_buffers_len(encoder.encode(&batch).unwrap()); | ||
| } | ||
| encoded_len += encoded_buffers_len(encoder.finish().unwrap()); | ||
| black_box(encoded_len); |
There was a problem hiding this comment.
wouldn't we want to call black_box() on encoder.encode()? why are we calling it on the final computed length?
There was a problem hiding this comment.
Good point. The final length prevents the aggregate from being optimized away, but black-boxing the encoded buffers is a better benchmark boundary. I’ll update the StreamEncoder cases to black-box the encode / finish results before computing the length. Done in 868d643
There was a problem hiding this comment.
so do we still need to accumulate the lengths like this, or black boxing the encode methods is already sufficient?
There was a problem hiding this comment.
Yeah, that makes sense. Once the encoded buffers are passed through black_box, the length accumulation is redundant and adds extra work to the benchmark. I’ll simplify this to black-box the encode / finish results directly.
Black-box StreamEncoder benchmark outputs before measuring their lengths so the benchmark boundary is less fragile. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
Black-boxing the StreamEncoder outputs is enough to keep the work observable. Avoid traversing buffer lengths in the measured path. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
Jefffrey
added
the
development-process
|
thanks @Phoenix500526 & @Rich-T-kid |
3 participants
Which issue does this PR close?
Rationale for this change
Add a
StreamEncoderbenchmarks so that we can catch regressions without measuring sink IO.What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No