perf/bug: use accurate memory size when determining flight batch chunks - #10383
perf/bug: use accurate memory size when determining flight batch chunks#10383Rich-T-kid wants to merge 5 commits into
Conversation
fn compare_column_sizes(batch: &RecordBatch) {
let schema = batch.schema();
for (i, col) in batch.columns().iter().enumerate() {
let name = schema.field(i).name();
let data = col.to_data();
let slice_size = data.get_slice_memory_size().unwrap();
let buffer_size = col.get_buffer_memory_size();
if slice_size != buffer_size {
println!(
"[{name}] data_type={:?} slice_size={slice_size} buffer_size={buffer_size} diff={}",
data.data_type(),
buffer_size as i64 - slice_size as i64
);
} else {
println!(
"[{name}] data_type={:?} sizes match ({slice_size})",
data.data_type()
);
}
}
}is resulting in [s1] data_type=Utf8 slice_size=14336 buffer_size=20544 diff=6208
[i1] data_type=Int16 sizes match (2048)
[s2] data_type=Utf8 slice_size=10240 buffer_size=12352 diff=2112
[i2] data_type=Int64 sizes match (8192)the calculations for the string arrays are not accurate. I could fix the breaking test |
|
to keep this PR lean im going to resolve the inaccurate results produced by |
|
Spent a few hours debugging this. going to leave this comment sharing what I found in case anyone has ideas on how to improve this. The PR is mostly ready for review so mark it as open. Previously, the size estimate used for chunking was often much larger than the actual memory the RecordBatch held. Since n_batches = size / target_size_chunks, that inflated size produced more chunks than necessary. Take a RecordBatch of size The current implementation looks at the full This happens to work today because This works in the test's favor right now because a sliced RecordBatch's reported size is roughly 2x larger than what it actually takes up once IPC-encoded. That ~2x gap is the only reason the test currently passes. With the switch to get_slice_memory_size, the size returned now reflects the actual memory the sliced batch takes up, so the calculation correctly figures out almost the exact number of rows needed to fill a max_flight_data_size chunk. off the top of my head it seems theres two viable solutions
cc @milenkovicm |
|
I may also just be missing something, the comment above |
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes #10385. - works towards closing #10383 # Rationale for this change `get_slice_memory_size` is used to estimate how much memory a slice of an ArrayData actually occupies. For variable-width types (Utf8, Binary, List, etc.), the offset buffer stores len+1 boundaries, one per element plus a final entry marking the end of the last element. The old code calculated `len * byte_width`, missing that final boundary and consistently under-reporting the size of any array using an offset buffer. <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> # What changes are included in this PR? the FixedWidth buffer size is now calculated as (len + 1) * byte_width instead of len * byte_width. <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> # Are these changes tested? Yes I added two test for string arrays and binary arrays. existing test still pass <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> # Are there any user-facing changes? users will have more accurate memory accounting <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
98684bf to
8500b43
Compare
|
still a wip, added a couple test for the reproducers mentioned in #9388 |
Which issue does this PR close?
split_batch_for_grpc_responseover-splits batches after IPC deserialization #9388.get_slice_memory_sizeor similar #3407Rationale for this change
TODO
see #9388.
What changes are included in this PR?
replaces
get_buffer_memory_size()withto_data().get_slice_memory_size(). This is to get a more accurate size of record batches being sent over the wire.Are these changes tested?
yes, there is a unit test included in this PR. the test asserts that a few number of slices are produced from
split_batch_for_grpc_responsewhen the arrays passed to it are smaller.Are there any user-facing changes?
no