Improve perfomance of reverse function
#14025
Conversation
Signed-off-by: Tai Le Manh <[email protected]>
|
|
||
| for string in string_array.iter() { | ||
| if let Some(s) = string { | ||
| let mut reversed = String::with_capacity(s.len()); |
There was a problem hiding this comment.
I wonder if this allocation can be removed by using the Write impl? See https://arrow.apache.org/rust/arrow/array/type.GenericStringBuilder.html#example-incrementally-writing-strings-with-stdfmtwrite
Perhaps by iterating through the rev iterator, writing chars one at a time.
If the above is slower, it could also be interesting to see if reusing the String allocation with a clear() on every loop is faster
There was a problem hiding this comment.
@simonvandel Thanks for reviewing
I wonder if this allocation can be removed by using the Write impl? See https://arrow.apache.org/rust/arrow/array/type.GenericStringBuilder.html#example-incrementally-writing-strings-with-stdfmtwrite
Perhaps by iterating through the rev iterator, writing chars one at a time.
I tested this solution and the performance is not as good as this PR.
If the above is slower, it could also be interesting to see if reusing the String allocation with a clear() on every loop is faster
Indeed, this one is faster. I will update the code and provide benchmarks shortly. TYSM ❤️
| .map(|string| string.map(|string: &str| string.chars().rev().collect::<String>())) | ||
| .collect::<GenericStringArray<T>>(); | ||
| let mut builder: GenericStringBuilder<T> = | ||
| GenericStringBuilder::with_capacity(string_array.len(), 1024); |
There was a problem hiding this comment.
I think we can use the actual data size here for pre-allocation, instead of a constant 1024, the complexity of adding another argument for array size seems reasonable
There was a problem hiding this comment.
@2010YOUY01 Thanks for reviewing,
I think we can use the actual data size here for pre-allocation, instead of a constant 1024, the complexity of adding another argument for array size seems reasonable
I agree that it would be better if we could pre-allocate the actual data size here, but I think it's difficult to compute accurately - it depends on context. Keeping it simple here seems reasonable as well.
Currently GenericStringBuilder have new and with_capacity to init new builder, and 1024 is default size if we using GenericStringBuilder::new (ref) that's why I choose 1024 here.
There was a problem hiding this comment.
I wonder if this is a good alternative to 1024? 🤔
There was a problem hiding this comment.
I wonder if this is a good alternative to
1024? 🤔
Yes, I think so
There was a problem hiding this comment.
Yes, I think so
Unfortunately, it's a bit slower than the current implementation. It allocates more mem than I expected, string_array.get_array_memory_size() = 16632 when str_len = 1024, and = 66168 when str_len = 4096 🤔
There was a problem hiding this comment.
I noticed get_array_memory_size() will overestimate source, also many other string function is not using the accurate estimation
I think it's okay to keep it simple and use the default size now, perhaps we can introduce a function to calculate only payload size in the future, and make estimation correct for all usages of GenericStringBuilder. Thank you for the experiment.
|
Nice work @tlm365 @2010YOUY01 and @simonvandel ❤️ |
|
🚀 |
Which issue does this PR close?
Closes #.
Rationale for this change
Improve performance of
reversefunction.What changes are included in this PR?
Are these changes tested?
By CI.
Are there any user-facing changes?
No.
*Benchmark result