-
Notifications
You must be signed in to change notification settings - Fork 1.9k
perf: improve performance of string replace #19530
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
Conversation
3e29ea3 to
3f8c9bb
Compare
Use a reusable String buffer instead of allocating a new String for each row. This optimization achieves 17-32% performance improvement across different string types and sizes by avoiding per-row allocations. Benchmark results: | Benchmark | Array Size | String Length | Baseline (µs) | Optimized (µs) | Improvement | |---------------|------------|---------------|---------------|----------------|-------------| | string_view | 1024 | 32 | 32.53 | 22.32 | 31.4% faster| | string | 1024 | 32 | 31.89 | 21.49 | 32.6% faster| | large_string | 1024 | 32 | 31.75 | 22.01 | 30.7% faster| | string_view | 1024 | 128 | 49.51 | 36.11 | 27.1% faster| | string | 1024 | 128 | 48.91 | 34.90 | 28.6% faster| | large_string | 1024 | 128 | 49.78 | 35.42 | 28.8% faster| | string_view | 4096 | 32 | 133.67 | 95.93 | 28.2% faster| | string | 4096 | 32 | 131.48 | 91.73 | 30.2% faster| | large_string | 4096 | 32 | 129.61 | 92.82 | 28.4% faster| | string_view | 4096 | 128 | 191.50 | 153.74 | 19.7% faster| | string | 4096 | 128 | 185.27 | 149.37 | 19.4% faster| | large_string | 4096 | 128 | 187.82 | 154.32 | 17.8% faster|
3f8c9bb to
c10d410
Compare
| Ok(Arc::new(result) as ArrayRef) | ||
| /// Helper function to perform string replacement into a reusable String buffer | ||
| #[inline] | ||
| fn replace_into_string(buffer: &mut String, string: &str, from: &str, to: &str) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we lost other optimization tricks that the Rust replace function provides; for example looking at the code: https://doc.rust-lang.org/src/alloc/str.rs.html#268
It has a Fast path for replacing a single ASCII character with another. I wonder if we capture this in our benchmarks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent point! Thanks for the review. The single ASCII character fast path optimization has been added.
abdede8 to
39ae000
Compare
Add a fast path for replacing single ASCII characters with another single ASCII character, matching Rust's str::replace() optimization. This enables vectorization and avoids UTF-8 boundary checking overhead. Changes: - Added ASCII character detection in replace_into_string() - When both 'from' and 'to' are single ASCII bytes, use direct byte mapping - Updated benchmark to include single ASCII character replacement tests Optimization: - Fast path operates directly on bytes using simple map operation - Compiler can vectorize the byte-wise replacement - Avoids overhead of match_indices() pattern matching for this common case Benchmark Results (Single ASCII Character Replacement) against previous commit: - size=1024, str_len=32: 29.5 µs → 21.4 µs (27% faster) - size=1024, str_len=128: 73.9 µs → 23.4 µs (68% faster) - size=4096, str_len=32: 121.8 µs → 85.6 µs (30% faster) - size=4096, str_len=128: 316.9 µs → 83.8 µs (74% faster) The optimization shows exceptional 27-74% improvements, with the benefit scaling dramatically with string length. For 128-character strings, we achieve over 3x speedup by enabling vectorization and eliminating pattern matching overhead. This addresses reviewer feedback about capturing Rust's str::replace() optimization tricks for single ASCII character replacements.
39ae000 to
4f36125
Compare
andygrove
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice speedups! Thanks @viirya.
|
Thanks @andygrove @Jefffrey |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Use a reusable String buffer instead of allocating a new String for each row. This optimization achieves 17-32% performance improvement across different string types and sizes by avoiding per-row allocations.
Benchmark results:
Are these changes tested?
Are there any user-facing changes?