perf: optimize left function by eliminating double chars() iteration #19571
+12
−3
Conversation
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
viirya
force-pushed
the
left_optimize
branch
from
413e195 to
08f3cdb
Compare
Contributor
|
Can you make a PR with just the benchmarks (which is easier to merge) so we can then compare main vs. changed? |
Member
Author
|
Opened #19600. |
martin-g
reviewed
Jan 2, 2026
| chars_buf.extend(string.chars()); | ||
| let len = chars_buf.len() as i64; | ||
|
|
||
| Some(if n.abs() < len { |
Member
There was a problem hiding this comment.
minor improvement: calling .abs() on MIN will lead to problems (crash in dev and wrapping in release).
i64::MIN is a corner case, so I guess no one will complain that it is not supported here.
I also tried with if n + len > 0 but then it was 3% slower on my machine.
Suggested change
| Some(if n.abs() < len { | |
| Some(if n != i64::MIN && n.abs() < len { |
Member
Author
There was a problem hiding this comment.
Replaced n.abs() < len with n > -len to avoid panic/wrap on i64::MIN now.
For negative n values, the function was calling string.chars() twice: 1. Once to count total characters 2. Again to take the prefix This optimization collects chars into a reusable buffer once per row for the negative n case, eliminating the redundant iteration. Benchmark results (negative n, which triggers the optimization): - size=1024: 71.323 µs → 52.760 µs (26.0% faster) - size=4096: 289.62 µs → 212.23 µs (26.7% faster) Benchmark results (positive n, minimal overhead): - size=1024: 24.465 µs → 24.691 µs (0.9% slower) - size=4096: 96.129 µs → 97.078 µs (1.0% slower) The dramatic improvement for negative n cases far outweighs the negligible overhead for positive n cases.
Replaced `n.abs() < len` with `n > -len` to avoid panic/wrap on i64::MIN. The original code used .abs() on a negative i64 value, which causes: - Panic in debug builds when n = i64::MIN (overflow check) - Wrapping behavior in release builds (undefined behavior) The new condition `n > -len` is mathematically equivalent: - When n < 0: n.abs() = -n - So: -n < len is equivalent to n > -len This handles the i64::MIN edge case safely without performance impact, as it's just a comparison with negated value instead of abs(). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
For negative n values, the function was calling string.chars() twice:
This optimization collects chars into a reusable buffer once per row for the negative n case, eliminating the redundant iteration.
Benchmark results (negative n, which triggers the optimization):
Benchmark results (positive n, minimal overhead):
The dramatic improvement for negative n cases far outweighs the negligible overhead for positive n cases.
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?