-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize performance of the substr_index function #9890
Comments
Hi, I was curious about this and decided to test it out myself. I first generated a flamegraph using flamegraph-rs ( It seems like quite a lot of time is spent on A snippet of flamegraph.svg: I then tested out a quick change to avoid the % cargo bench --bench substr_index -- --baseline main
Compiling datafusion-functions v37.0.0 (/Users/kevin/git/arrow-datafusion/datafusion/functions)
Finished bench [optimized] target(s) in 56.31s
Running benches/substr_index.rs (target/release/deps/substr_index-bcc13e06371405f5)
substr_index_array_array_1000
time: [87.912 µs 88.197 µs 88.515 µs]
change: [-29.549% -28.257% -27.292%] (p = 0.00 < 0.05)
Performance has improved. diff: - let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
- Box::new(string.split(delimiter))
- } else {
- Box::new(string.rsplit(delimiter))
- };
let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
- // The length of the substring covered by substr_index.
- let length = splitted
- .take(occurrences) // at least 1 element, since n != 0
- .map(|s| s.len() + delimiter.len())
- .sum::<usize>()
- - delimiter.len();
+ let length;
+ if n > 0 {
+ let splitted = string.split(delimiter);
+ length = splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len();
+ } else {
+ let splitted = string.rsplit(delimiter);
+ length = splitted
+ .take(occurrences)
+ .map(|s| s.len() + delimiter.len())
+ .sum::<usize>()
+ - delimiter.len();
+ } |
🚀 |
FYI @Omega359 |
The 25-30% improvement is in line with what I expected for the box removal - very nice work! |
I'll make a PR some time today or tmr! |
take |
Is your feature request related to a problem or challenge?
As noted in #9864 (comment) there might be some optimization available for the substr_index function so to facilitate that add a benchmark for that function.
A new benchmark testing the substr_index function was added in #9878 and available for use to test the performance of that function and any changes.
Describe the solution you'd like
Make the
substr_index
benchmark fasterTo run:
Describe alternatives you've considered
No response
Additional context
No response
The text was updated successfully, but these errors were encountered: