Repository navigation
perf: Optimize left, right
#26039
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
perf: Optimize left, right
#26039
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,9 +17,9 @@ | |
|
|
||
| //! Common utilities for implementing unicode functions | ||
|
|
||
| use crate::strings::{MAX_INLINE_LEN, sub_view}; | ||
| use arrow::array::{ | ||
| Array, ArrayRef, ByteView, GenericStringArray, Int64Array, OffsetSizeTrait, | ||
| StringViewArray, make_view, | ||
| Array, ArrayRef, GenericStringArray, Int64Array, OffsetSizeTrait, StringViewArray, | ||
| }; | ||
| use arrow::datatypes::DataType; | ||
| use arrow_buffer::{NullBuffer, ScalarBuffer}; | ||
|
|
@@ -126,21 +126,34 @@ pub(crate) enum StringCharLen { | |
| #[inline] | ||
| fn left_right_byte_length(string: &str, n: i64) -> usize { | ||
| let abs = n.unsigned_abs().min(usize::MAX as u64) as usize; | ||
| // For ASCII input every character is exactly one byte, so the byte offset of | ||
| // the n-th codepoint is just the (clamped) character count. This avoids the | ||
| // per-character `char_indices()` scan of the general path. | ||
| let bytes = string.as_bytes(); | ||
| // ASCII bytes are never part of a multi-byte UTF-8 sequence, so if the | ||
| // `abs` bytes at the relevant end of the string are ASCII, they are exactly | ||
| // the `abs` characters at that end. Checking only those bytes is cheaper | ||
| // than either a `char_indices()` scan or checking the whole string. | ||
| match n.cmp(&0) { | ||
| Ordering::Equal => 0, | ||
| // `abs` chars trimmed from the end: keep the leading `len - abs`. | ||
| Ordering::Less if string.is_ascii() => string.len().saturating_sub(abs), | ||
| Ordering::Less => string | ||
| .char_indices() | ||
| .nth_back(abs - 1) | ||
| .map(|(index, _)| index) | ||
| .unwrap_or(0), | ||
| // First `abs` chars, but never past the end of the string. | ||
| Ordering::Greater if string.is_ascii() => abs.min(string.len()), | ||
| Ordering::Greater => byte_offset_of_char(string, abs), | ||
| // Byte offset of the `abs`-th character from the end. | ||
| Ordering::Less => { | ||
| let start = bytes.len().saturating_sub(abs); | ||
| if bytes[start..].is_ascii() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When An early exit per arm would skip it, for example
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| start | ||
| } else { | ||
| string | ||
| .char_indices() | ||
| .nth_back(abs - 1) | ||
| .map_or(0, |(index, _)| index) | ||
| } | ||
| } | ||
| // Byte offset of the `abs`-th character from the start. | ||
| Ordering::Greater => { | ||
| let end = abs.min(bytes.len()); | ||
| if bytes[..end].is_ascii() { | ||
| end | ||
| } else { | ||
| byte_offset_of_char(string, abs) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -204,14 +217,10 @@ fn general_left_right_view<F: LeftRightSlicer>( | |
| let n = n_array.value(idx); | ||
|
|
||
| let range = F::slice(string, n); | ||
| let result_bytes = &string.as_bytes()[range.clone()]; | ||
| if result_bytes.len() > 12 { | ||
| if range.len() > MAX_INLINE_LEN { | ||
| has_out_of_line = true; | ||
| } | ||
|
|
||
| let byte_view = ByteView::from(views[idx]); | ||
| let new_offset = byte_view.offset + (range.start as u32); | ||
| make_view(result_bytes, byte_view.buffer_index, new_offset) | ||
| sub_view(views[idx], string.as_bytes(), range) | ||
| }) | ||
| .collect::<Vec<u128>>(); | ||
|
|
||
|
|
@@ -223,7 +232,10 @@ fn general_left_right_view<F: LeftRightSlicer>( | |
| }; | ||
|
|
||
| // SAFETY: | ||
| // - Each view is produced by `make_view` with correct bytes and offset | ||
| // - Each view is produced by `sub_view` from the input view and a range | ||
| // within the input string, as returned by `F::slice` | ||
|
neilconway marked this conversation as resolved.
|
||
| // - `F::slice` returns ranges that start and end on char boundaries (see | ||
| // `left_right_byte_length`), so every result is valid UTF-8 | ||
| // - Out-of-line views reuse the original buffer index and adjusted offset | ||
| unsafe { | ||
| let array = StringViewArray::new_unchecked(views, data_buffers, new_nulls); | ||
|
|
||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same logic also lives as a private
substr_viewinstring/split_part.rs:509andunicode/substrindex.rs:583. Neither callsappend_view, so the follow-up can retire three definitions.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.
Yep, makes sense!
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.
FYI, the PR for replacing these other definitions with
sub_viewis here: #26121