Repository navigation
perf: Optimize left, right - #26039
Conversation
|
FYI @andygrove @comphead |
| pub(crate) fn sub_view(view: u128, source: &[u8], range: Range<usize>) -> u128 { | ||
| debug_assert!(range.start <= range.end && range.end <= source.len()); | ||
|
|
||
| // The substring's first bytes, in the low-order bits. Any bits past the | ||
| // end of the substring are masked off by `inline_view`. | ||
| let leading_bytes = if source.len() <= MAX_INLINE_LEN { | ||
| // `source` is stored in `view` itself, after its 4-byte length. | ||
| (view >> 32) >> (8 * range.start) | ||
| } else { | ||
| // `source` has more than 12 bytes, so read the 12 bytes starting at | ||
| // `range.start`, or the last 12 bytes if that would run past the end, | ||
| // and skip any that come before `range.start`. | ||
| let window_start = range.start.min(source.len() - MAX_INLINE_LEN); | ||
| let window = source[window_start..window_start + MAX_INLINE_LEN] | ||
| .try_into() | ||
| .unwrap(); | ||
| read_12_bytes(window) >> (8 * (range.start - window_start)) | ||
| }; | ||
|
|
||
| let len = range.len(); | ||
| if len <= MAX_INLINE_LEN { | ||
| inline_view(leading_bytes, len) | ||
| } else { | ||
| let original = ByteView::from(view); | ||
| ByteView { | ||
| length: len as u32, | ||
| prefix: leading_bytes as u32, | ||
| offset: original.offset + range.start as u32, | ||
| ..original | ||
| } | ||
| .as_u128() | ||
| } | ||
| } |
There was a problem hiding this comment.
This could potentially be used in other places (e.g., substr), but that will require more careful evaluation; I'll defer that for now.
There was a problem hiding this comment.
The same logic also lives as a private substr_view in string/split_part.rs:509 and unicode/substrindex.rs:583. Neither calls append_view, so the follow-up can retire three definitions.
There was a problem hiding this comment.
Yep, makes sense!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26039 +/- ##
========================================
Coverage 82.66% 82.67%
========================================
Files 1147 1147
Lines 446357 446589 +232
Branches 446357 446589 +232
========================================
+ Hits 368971 369205 +234
+ Misses 54997 54981 -16
- Partials 22389 22403 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
apache#23762 added an ASCII fast path to `left_right_byte_length` that calls `is_ascii()` on the whole string for every row. For short results from longer strings, such as Utf8View data read from Parquet, that scan costs more than the per-character scan it replaced. ASCII bytes are never part of a multi-byte UTF-8 sequence, so if the `n` bytes at the relevant end of the string are ASCII, they are exactly the `n` characters at that end. Check only those bytes, and fall back to the per-character scan otherwise.
`make_view` selects per-length copy code with a jump on the result length. When result lengths vary from row to row, as with a negative `n`, the CPU often mispredicts that jump. Checking only the result bytes for ASCII removed a length-dependent loop that had been making the jump predictable, so those cases got slower. Add `sub_view`, which builds the view for a substring of an existing view with shifts and masks: from the view itself when the source is inlined, and otherwise from a 12-byte window of the source that is always in bounds. Use it for Utf8View input in `left` and `right`.
b3e5014 to
3a137c2
Compare
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @neilconway , overall LGTM
comphead
left a comment
There was a problem hiding this comment.
Thanks @neilconway makes a lot of sense for me
comphead
left a comment
There was a problem hiding this comment.
Non-blocking notes:
- Bench coverage:
left_right.rsbuilds its strings with arrow's alphanumeric generator, so every value is ASCII. The multibyte fallback and the "ASCII end, multibyte elsewhere" shortcut are not measured. Two small cases would cover both: a multibyte char inside then-char window (fallback) and one only outside it (shortcut). inline_viewhas a single caller and itsdebug_assert!repeats thelen <= MAX_INLINE_LENguard right above the call. Folding it into that arm would drop both.
| // Byte offset of the `abs`-th character from the end. | ||
| Ordering::Less => { | ||
| let start = bytes.len().saturating_sub(abs); | ||
| if bytes[start..].is_ascii() { |
There was a problem hiding this comment.
When abs >= bytes.len() no byte needs inspecting. The result is 0 here (bytes.len() in the Greater arm) whatever the content, since a string has at most bytes.len() chars. Today that case scans the whole string with is_ascii, and non-ASCII input then also pays a full char_indices pass to reach the same value. That matches n_exceeds_len being flat for Utf8 in the table (-1% to +3%) while short_result_long_input drops 35% to 71%.
An early exit per arm would skip it, for example start == 0 || bytes[start..].is_ascii() here and end == bytes.len() || bytes[..end].is_ascii() below. It adds a length-dependent branch, so short_result and per_row_n, where only some rows have abs >= len, are worth rechecking next to n_exceeds_len. I have not measured any of this.
There was a problem hiding this comment.
I agree that that could be a win, but it merits separate evaluation / benchmarking. I'd rather land this and then take a look at that subsequent optimization in a followup.
|
On the other comments @comphead:
|
|
Thanks for the reviews @jayzhan211 @comphead ! |
Which issue does this PR close?
Rationale for this change
#23762 added an ASCII fast-path for
leftandright. That improved performance in many scenarios, but the implementation callsis_asciion every input string. That is expensive, particularly for the common case thatleftandrightare used to compute a small prefix/suffix of a much longer string. That check is also overly conservative: for example, we can take the ASCII fast-path forleft(s, k)if the firstkbytes in the string are ASCII, even if there are multibyte characters elsewhere in the string.This PR implements two optimizations:
is_asciion the bytes necessary to determine if we can take the fast-path, not the entire input string, as described above.Utf8Viewinputs, the first optimization regressed some benchmark cases (e.g.,negative_n) on both ARM and x86. Claude's theory is thatmake_viewis out-of-line and does an indirect jump on the length of the result string; it seems that after implementing the first optimization, this jump was not well-handled by the branch predictor. Instead, we add a helpersub_viewthat returns a view that is a substring of an existing view. This can be inlined and avoids the indirect jump incurred bymake_view; it can also construct the new view from the old view with bitwise ops, rather than building the new view on the stack.Benchmarks:
x86 (AMD EPYC Milan)
ARM (Apple M4 Max)
What changes are included in this PR?
See above.
What is the testing strategy for this PR?
Existing tests pass; no functional changes.
Are there any user-facing changes?
No.