perf: speed up substring_by_char with an ASCII fast path and single-pass bounds#10334
perf: speed up substring_by_char with an ASCII fast path and single-pass bounds#10334andygrove wants to merge 1 commit into
Conversation
…ass bounds Avoid the unconditional chars().count() on every value, use nth_back for negative starts, and compute byte bounds arithmetically when the array is all ASCII.
|
I can take a look at this tomorrow morning. |
Jefffrey
left a comment
There was a problem hiding this comment.
running the benchmarks locally, great results
substring utf8 (start = 0, length = None)
time: [2.5852 ms 2.5907 ms 2.5969 ms]
change: [+0.2196% +0.5704% +0.9392%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 14 outliers among 100 measurements (14.00%)
11 (11.00%) low mild
1 (1.00%) high mild
2 (2.00%) high severe
substring utf8 (start = 1, length = str_len - 1)
time: [3.5970 ms 3.6051 ms 3.6148 ms]
change: [+0.2273% +0.6151% +1.0087%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
1 (1.00%) high mild
7 (7.00%) high severe
substring utf8 by char time: [4.6956 ms 4.7021 ms 4.7096 ms]
+ change: [−87.045% −87.012% −86.979%] (p = 0.00 < 0.05)
Performance has improved.
Found 26 outliers among 100 measurements (26.00%)
7 (7.00%) low severe
3 (3.00%) low mild
6 (6.00%) high mild
10 (10.00%) high severe
substring by char (ascii, prefix)
time: [2.2969 ms 2.3041 ms 2.3126 ms]
- change: [+7.0461% +7.4037% +7.7906%] (p = 0.00 < 0.05)
Performance has regressed.
Found 20 outliers among 100 measurements (20.00%)
1 (1.00%) low mild
4 (4.00%) high mild
15 (15.00%) high severe
substring by char (ascii, tail)
time: [2.3013 ms 2.3083 ms 2.3167 ms]
+ change: [−93.238% −93.214% −93.188%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
2 (2.00%) high mild
6 (6.00%) high severe
Benchmarking substring by char (non-ascii, prefix): Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.6s, enable flat sampling, or reduce sample count to 60.
substring by char (non-ascii, prefix)
time: [1.0798 ms 1.0989 ms 1.1202 ms]
+ change: [−63.584% −63.142% −62.655%] (p = 0.00 < 0.05)
Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
9 (9.00%) high mild
1 (1.00%) high severe
substring by char (non-ascii, tail)
time: [812.63 µs 823.64 µs 837.11 µs]
+ change: [−98.695% −98.673% −98.649%] (p = 0.00 < 0.05)
Performance has improved.
Found 19 outliers among 100 measurements (19.00%)
18 (18.00%) high mild
1 (1.00%) high severe
Benchmarking substring fixed size binary array: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 6.9s, enable flat sampling, or reduce sample count to 60.
substring fixed size binary array
time: [1.3592 ms 1.3604 ms 1.3617 ms]
change: [−0.5443% −0.1591% +0.2104%] (p = 0.43 > 0.05)
No change in performance detected.
Found 13 outliers among 100 measurements (13.00%)
3 (3.00%) high mild
10 (10.00%) high severe- that regression is probably just noise on my machine 🤔
|
run benchmarks substring |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing optimize-substring-by-char (c708cd5) to 543b806 (merge-base) diff File an issue against this benchmark runner |
|
Benchmark for this request failed. Last 20 lines of output: Click to expandFile an issue against this benchmark runner |
Which issue does this PR close?
N/A — performance improvement to an existing kernel.
Rationale for this change
substring_by_charcallsval.chars().count()on every value, which is a full UTF-8 decode of the entire string. The result is only used whenstart < 0, so for the common non-negativestartcase the work is thrown away. Whenstart < 0, the string is then walked a second time from the front to convert the char index back into a byte offset.It also decodes UTF-8 unconditionally, even when the array is entirely ASCII, where a char index is a byte index and the bounds can be computed arithmetically. The doc comment currently tells users to reach for
substringthemselves in that case; the kernel can just detect it.This is a port of the approach taken in apache/datafusion-comet#4903, which replaced this kernel with a local copy for exactly these reasons.
What changes are included in this PR?
substring_by_charpicks a bounds function once per array:ascii_bounds(integer arithmetic on byte offsets) whenis_ascii()holds,utf8_boundsotherwise.substring_by_char_impldoes the shared buffer building.chars().count()is gone. Negative starts usechar_indices().nth_back(), which decodes only as far back as needed rather than scanning the whole value twice.utf8_boundsshort-circuits the end scan when the requested char length is at least the remaining byte length, since a char is never smaller than one byte.The output is still built with the checked
GenericStringArray::new. Skipping the redundant UTF-8 validation withnew_uncheckedis possible (every slice is on a char boundary of an already-valid string) but is left out of this PR.Benchmark, 65,536 rows x 1,000-char strings:
The ASCII prefix case gains the least because it is dominated by the copy and the output validation, but it still comes out ahead of the
is_ascii()scan it now pays for.Are these changes tested?
Yes. The existing
substring_by_chartests all mix ASCII and non-ASCII values in a single array, so they only ever exercised the UTF-8 path. Added an ASCII-only test matrix (ascii_string_by_char/ascii_large_string_by_char) covering identity, positive and negative starts, out-of-range starts in both directions, zero length, and au64::MAXlength to pin the saturating-add clamp.Added four benchmarks to
substring_kernels.rs(ASCII and non-ASCII, prefix and tail) plus a non-ASCII array generator; the existing bench name is unchanged so its history stays comparable.Are there any user-facing changes?
No API change.
substring_by_charis faster, and its# Performancedoc note is updated to mention the ASCII fast path.