Avoid redundant memory access in ArrowBytesViewMap for inline values#20807
Avoid redundant memory access in ArrowBytesViewMap for inline values#20807CuteChuanChuan wants to merge 5 commits intoapache:mainfrom
Conversation
CuteChuanChuan
force-pushed
the
raymond/issue-20054-avoid-extra-memory-access
branch
from
9330fd7 to
dd4cc0b
Compare
|
Hi @Dandandan , |
| // For non-inline strings (>12 bytes), fetch once from the array. | ||
| let view_bytes = view_u128.to_le_bytes(); | ||
| let input_value: &[u8] = if is_inline { | ||
| &view_bytes[4..4 + len as usize] |
There was a problem hiding this comment.
I think we don'nt need this for the inline path (we use the u128 directly)
| } else { | ||
| // to avoid redundant memory access during hash resolution and insertion. | ||
| // Inline strings (<= 12 bytes) use u128 directly. | ||
| let input_value: &[u8] = if !is_inline { |
There was a problem hiding this comment.
Hmm can we just values.value(i).as_ref() to where it is used instead in the else branch ?
There was a problem hiding this comment.
Done — use values.value(i).as_ref() to where it's used.
Sorry for overcomplicating it 😅
| let (payload, new_view) = if is_inline { | ||
| // Extract inline bytes from view (only for new values) | ||
| let view_bytes = view_u128.to_le_bytes(); | ||
| let payload = make_payload_fn(Some(&view_bytes[4..4 + len as usize])); |
There was a problem hiding this comment.
it looks like this param to the payload fn is not really used anymore, could we remove it?
The three actual implementations just ignore the param:
fn make_payload_fn(_value: Option<&[u8]>) {}
|_value| {
// assign new group index on each insert
let group_idx = self.num_groups;
self.num_groups += 1;
group_idx
},
|_value| {
// assign new group index on each insert
let group_idx = self.num_groups;
self.num_groups += 1;
group_idx
},
There was a problem hiding this comment.
Removed the parameter from make_payload_fn; updated callers and test accordingly.
- Remove precomputed input_value; call values.value(i) only where needed - Remove unused parameter from make_payload_fn and update relevant tests
|
Hi @Dandandan , just a gentle ping — could you take a look when you get a chance? |
|
Hi @Dandandan , |
…-avoid-extra-memory-access
3 participants
Which issue does this PR close?
Rationale for this change
ArrowBytesViewMap::insert_if_new_innercallsvalues.value(i)redundantly:findclosureFor inline strings (<=12 bytes),
values.value(i)is unnecessary since the data is embedded directly in the view.For non-inline strings (>12 bytes), it should only be called once.
What changes are included in this PR?
values.value(i). During insertion, push the originalview_u128directly.values.value(i)once before thefindclosure and reuse the result for both hash resolution and insertion.Are these changes tested?
Yes, covered by existing tests by running
cargo test -p datafusion-physical-expr-commonAre there any user-facing changes?
No. This is an internal performance optimization with no API changes.