fix[vortex-array]: fix rebuild_trim_elements overflow#6995
fix[vortex-array]: fix rebuild_trim_elements overflow#6995connortsui20 merged 1 commit intodevelopfrom
Conversation
This commit just casts offsets and sizes to u64/i64 before summing for simplicitly. We could make the logic more complicated but not sure it's worth it. Signed-off-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com>
Merging this PR will improve performance by 11.12%
Performance Changes
Comparing 
Footnotes
|
connortsui20
left a comment
connortsui20
left a comment
There was a problem hiding this comment.
I think this is probably fine given we narrow these in the compressor anyways. If for some reason this is bad (it probably isn't) then we can just do the next size up.
|
This shouldn't be relevant for the offsets/sizes width. This code path essentially only computes the "end" offset which is cast to a usize below to slice the elements. |
| PType::I64 | ||
| }); | ||
| let offsets = self.offsets().cast(wide_dtype.clone())?; | ||
| let sizes = self.sizes().cast(wide_dtype)?; |
There was a problem hiding this comment.
For types smaller than 64bit this will unconditionally widen => allocate a new buffer + do an element-wise copy.
There was a problem hiding this comment.
Yes, this is what I mean by "we could make the logic more complicated". To avoid the unconditional widening we would probably have to attempt the sum and then fall back to widening to the next largest width.
There was a problem hiding this comment.
Yeah I see, I guess we can act on this in case it shows up in perf profiles.
There was a problem hiding this comment.
Definitely! We're always profiling after all 😄
3 participants
This commit just casts offsets and sizes to u64/i64 before summing for simplicitly. We could make the logic more complicated but not sure it's worth it.
Summary
Closes: #6973
Testing
Regression test added