improve case conversion happy path by conradludgate · Pull Request #97046 · rust...
source link: https://github.com/rust-lang/rust/pull/97046
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
Someone shared the source code for Go's string case conversion.
It features a hot path for ascii-only strings (although I assume for reasons specific to go, they've opted for a read safe hot loop).
I've borrowed these ideas and also kept our existing code to provide a fast path + seamless utf-8 correct path fallback.
(Naive) Benchmarks can be found here https://github.com/conradludgate/case-conv
For the cases where non-ascii is found near the start, the performance of this algorithm does fall back to original speeds and has not had any measurable speed loss
added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label
Collaborator
rust-highfive commented 19 days ago
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
Collaborator
rust-highfive commented 19 days ago
(rust-highfive has picked a reviewer for you, use r? to override) |
added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label
This comment has been minimized.
Contributor
Author
conradludgate commented 17 days ago
I have benchmarked this on unicode heavy texts too (a 3MB Russian text file) and have found marginally better performance in this change:
So I am not worried about the initial ascii checks being negative to performance in strictly non-ascii contexts |
Could you maybe try German or something like that. And put umlauts somewhere at the end? (Or just use random garbled text where the start is only ASCII and the end contains unicode |
This is basically when my original benchmarks do. Features 2 copies of Macbeth, one untouched only ascii, the other features 16 bytes of non-ascii at the end. This was to test how well it handled pure ascii with the seamless break out:
Performance it expected to be somewhere in the middle if the text contains a unicode character half way through. Which is exactly what we see in the following results:
|
Contributor
Author
conradludgate commented 11 days ago
r? @thomcc |
Nice benches, but what about short strings? |
Fair point. While still maybe unnatural, I ran the following benchmark
over my large text files, this way it's tested more against shorter random length strings. I got the following results:
This most likely would improve by reducing the magic number Reducing the magic number from 16 to 8 I got minimal improvements (-25%) over the small string bench, but significant regression in the large string bench (+90%) |
Contributor
thomcc left a comment
This mostly looks okay to me, and I expect it to be an improvement, but I'm not sure that uppercasing strings over 100 bytes long is actually the common case.
Contributor
thomcc commented 7 days ago
Do you have benchmarks of the current version? |
Contributor
Author
conradludgate commented 7 days ago
Long ascii string
Long string with unicode half way through
Short ascii strings
|
This looks good to me, then. No rollup because of possible perf impact if things change cases. @bors r+ rollup=never |
Wait, maybe squash 10 commits? |
Contributor
klensy commented 7 days ago
bors sleep. |
Contributor
thomcc commented 7 days ago
@bors r+ rollup=never |
Contributor
bors commented 7 days ago
Commit d0f9930 has been approved by |
added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
labels
Contributor
bors commented 7 days ago
Contributor
bors commented 7 days ago
Test successful - checks-actions |
Collaborator
rust-timer commented 6 days ago
Finished benchmarking commit (1851f08): comparison url. Instruction count
Max RSS (memory usage)Results CyclesThis benchmark run did not return any relevant results for this metric. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes
|
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Successfully merging this pull request may close these issues.
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK