Improve memory usage for arrow-row -> String/BinaryView when utf8 validation disabled
#7917
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@alamb @XiangpengHao @2010YOUY01 When running the existing benchmarks ( |
|
I took a high level look and it looks good to me. Curious to see the perf diff. Does the benchmark report memory usage yet? @ding-young |
|
@XiangpengHao The current benchmark doesn’t report memory usage directly, but I’ve been printing stats manually using jemalloc. It seems like there might be an issue with my implementation, so I’ll double-check that and share the perf once I’ve confirmed. |
I added code to get jemalloc stats (allocate, resident, active) before and after decoding binary view, and the memory usage actually improved especially when short strings are mixed up with large strings. When given rows consists of only large strings, the memory usage was the same. let before = jemalloc_stat();
let view = if !validate_utf8 {
decode_binary_view_inner_utf8_unchecked(rows, options)
} else {
decode_binary_view_inner(rows, options, validate_utf8)
};
let after = jemalloc_stat();
// print ( after - before ) (To reproduce, see https://github.com/ding-young/arrow-rs/tree/issue-6057-bench-mem )
|
alamb
reviewed
Jul 22, 2025
There was a problem hiding this comment.
Thanks @ding-young -- the basic idea makes sense to me but this PR contains a lot of duplicated code which makes it hard to understand what is actually changing here
if the goal of this PR is to reduce the size of the values buffer when we are not validating utf8, I think that should also be testable
For example decode the same rows with and without validation and show the buffer without validation is smaller 🤔
arrow-row/src/variable.rs
Outdated
| @@ -246,10 +246,76 @@ pub fn decode_binary<I: OffsetSizeTrait>( | |||
| unsafe { GenericBinaryArray::from(builder.build_unchecked()) } | |||
| } | |||
|
|
|||
| fn decode_binary_view_inner( | |||
| fn decode_binary_view_inner_utf8_unchecked( | |||
There was a problem hiding this comment.
I don't understand -- if it is a BinaryViewArray it can never have utf8 data. Maybe we can rename this function
There was a problem hiding this comment.
decode_string_view also calls decode_binary_view_inner(...), so this function can still be reached when decoding UTF-8 data. I'll think about whether there’s a clearer way to rename it.
ding-young
commented
Jul 25, 2025
| check_utf8: bool, | ||
| validate_utf8: bool, |
There was a problem hiding this comment.
Just to keep the naming consistent with
arrow-rs/arrow-row/src/variable.rs
Lines 343 to 346 in 16794ab
|
@alamb Thank you for review! I've added tests to compare the length of values buffer, and moved branching logic into |
alamb
approved these changes
Jul 28, 2025
There was a problem hiding this comment.
Thank you @ding-young -- I reviewed this code carefully and it makes sense to me. I have also started the benchmarks.
My scripts won't include your new benchmarks as they are in the same PR. If you can break the benchmarks into a separate PR that would be much easier for me to check
|
🤖 |
|
🤖: Benchmark completed Details
|
I opened a separate PR, thanks :) |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
benchmark results look good -- no regressions |
|
Thanks again @ding-young |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
As described in above issue, when constructing a
StringViewArrayfrom rows, we currently store inline strings twice: once throughmake_view, and again in thevalues bufferso that we can validate utf8 in one go. However, this is suboptimal in terms of memory consumption, so ideally, we should avoid placing inline strings into the values buffer when UTF-8 validation is disabled.What changes are included in this PR?
When UTF-8 validation is disabled, this PR modifies the string/bytes view array construction from rows as follows:
The capacity of the values buffer is set to accommodate only long strings plus 12 bytes for a single inline string placeholder.
All decoded strings are initially appended to the values buffer.
If a string turns out to be an inline string, it is included via
make_view, and then the corresponding inline portion is truncated from the values buffer, ensuring the inline string does not appear twice in the resulting array.Are these changes tested?
fuzz_testto set disable utf8 validation.Are there any user-facing changes?
No.
Considered alternatives
One idea was to support separate buffers for inline strings even when UTF-8 validation is enabled. However, since we need to call
decoded_len()first to determine the target buffer, this approach can be tricky or inefficient:For example, precomputing a boolean flag per string to determine which buffer to use would increase temporary memory usage.
Alternatively, appending to the values buffer first and then moving inline strings to a separate buffer would lead to frequent memcpy overhead.
Given that datafusion disables UTF-8 validation when using RowConverter, this PR focuses on improving memory efficiency specifically when validation is turned off.