perf: Optimize strpos() for ASCII-only inputs #20295
Open
+51
−32
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
Contributor
Author
|
Benchmark results: |
Contributor
Author
|
I added some notes on the approach and some future possible improvements to #20294 20294 |
Contributor
|
It would be great if you could use the PR template with relevant details, it maintains consistency. |
Contributor
Author
Sure. I've been removing sections that would otherwise have been left empty, but I can leave the full template in if you'd prefer. |
The previous implementation had a fast path for ASCII-only inputs, but it was still relatively slow. Switch to using memchr::memchr() to find the first matching byte and then check the rest of the bytes by hand. This improves performance for ASCII inputs by 2x-4x on the built-in strpos benchmarks.
neilconway
force-pushed
the
neilc/optimize-strpos
branch
from
1cb12b8 to
7e1ef9f
Compare
2010YOUY01
approved these changes
Feb 12, 2026
Contributor
2010YOUY01
left a comment
2010YOUY01
left a comment
There was a problem hiding this comment.
Nice optimization!
Left an idea for potential simplification, if it's slower, we can proceed with the current implementation.
| /// `memchr` does not, and strpos is often invoked many times on short inputs. | ||
| /// Returns a 1-based position, or 0 if not found. | ||
| /// Both inputs must be ASCII-only. | ||
| fn find_ascii_substring(haystack: &[u8], needle: &[u8]) -> usize { |
Contributor
There was a problem hiding this comment.
Is it possible to use memchr::memmem::find() directly? Based on the Complexity section, it seems has implemented the same algorithm.
https://docs.rs/memchr/latest/memchr/memmem/fn.find.html
Contributor
Author
There was a problem hiding this comment.
Thanks for the suggestion! When I tried using memmem::find(), it was substantially slower -- presumably because it incurs some per-call overhead (I'd imagine setting up lookup tables etc.) that memchr does not.
I'd like to explore optimizing the (common) case where strpos() is invoked with a constant substring; in that case we could construct a memmove::Finder once, and use it for the entire input batch. But this PR is already a significant win so my thought was to defer that to a subsequent PR.
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.
The previous implementation had a fast path for ASCII-only inputs, but it was still relatively slow. Switch to using memchr::memchr() to find the first matching byte and then check the rest of the bytes by hand. This improves performance for ASCII inputs by 2x-4x on the built-in strpos benchmarks.
Which issue does this PR close?
Are these changes tested?
Yes, passes unit tests and SLT.
Are there any user-facing changes?
No.