perf: Optimize left, right - #26039
Open
neilconway wants to merge 3 commits into
Open
neilconway wants to merge 3 commits into
neilconway wants to merge 3 commits into
Conversation
The Utf8 inputs were all exactly 32 bytes, while the Utf8View inputs used a different length distribution, so the two array types were not benchmarked on the same data. Fixed-length inputs also make per-row work that depends on the input length, such as an ASCII check over the whole string, perfectly predictable, so it looks nearly free. And every case passed `n` as an array cycling through a short range, although `n` is usually a literal. Generate variable-length inputs for every case and build the Utf8 and Utf8View arrays from the same strings. Pass `n` as a scalar, and add a case with a different random `n` per row. Add cases for short results from long inputs, for `n` exceeding the input length, and for negative `n`. Also derive the return field from the function instead of always using Utf8View. Since apache#23330, `left` and `right` return Utf8 for Utf8 input, and the mismatch fails the check in `ScalarUDF::invoke_with_args` when debug assertions are enabled.
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`.
Contributor
Author
|
FYI @andygrove @comphead |
neilconway
commented
Oct 4, 2026
Comment on lines
+1239
to
+1271
| 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() | ||
| } | ||
| } |
Contributor
Author
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26039 +/- ##
========================================
Coverage 82.65% 82.66%
========================================
Files 1147 1147
Lines 446087 446404 +317
Branches 446087 446404 +317
========================================
+ Hits 368721 369010 +289
- Misses 54980 55001 +21
- Partials 22386 22393 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
#23762 added an ASCII fast-path for
leftandright. That improves performance in many scenarios, but the implementation callsis_asciion every input string. That is expensive, particularly for the common case thatleftandrightare used to fetch a small prefix/suffix from 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: (separate PR #26026)
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.