-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Compile unicode-normalization
faster
#97936
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
Compile unicode-normalization
faster
#97936
Conversation
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit ca5e9908fdecd0a6f37dccb54fb0b3e8c416982b with merge dfb5ac9ecb0814d8c8d7f4ea4c0ee061170bdf97... |
☀️ Try build successful - checks-actions |
Queued dfb5ac9ecb0814d8c8d7f4ea4c0ee061170bdf97 with parent 420c970, future comparison URL. |
Finished benchmarking commit (dfb5ac9ecb0814d8c8d7f4ea4c0ee061170bdf97): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Footnotes |
|
||
// This code is hot when compiling `unicode-normalization` because it has a | ||
// number of matches with many ranges such as '\u{037A}'..='\u{037F}'. So | ||
// we special-case comparisons of chars in the relevant form for speed. |
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.
I dislike special casing chars just for one benchmark. I think we should generally improve the logic inside the eval_bits
and related functions to have a fast path for already evaluated constants.
For something that is actionable in this PR, please just replace this specific type match with one that performs this fast path for everything but floats and signed ints (or possibly pull the large type match below into a separate inline-always function and call it whenever both constants are already evaluated, irrespective of the type.
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.
Creating a fast path like that is tricky, because there are so many layers to these types, and because of the size checking and layout computations that takes place.
The fast path I added to this function hard to integrate with the rest of the function, because the fast path comparison is on ScalarInt
values, while the general case comparison is on u128
values. Extracting those u128
values in the fast case would require extra size checks and layout computations, eating into the speed wins. But I have generalized it to work with any scalar type other than Float
or Int
.
This is a performance win for `unicode-normalization`. Also, I find the new formulation easier to read.
This is a performance win for `unicode-normalization`. The commit also removes the closure, which isn't necessary. And reformulates the comparison into a form I find easier to read.
Also, the `try_to_bits` always succeeds, so use `unwrap`.
It's never executed when running the entire test suite. I think it's because of the early return at the top of the function if `a.ty() != ty` succeeds.
Because they're always equal.
A direct comparison has the same effect. This also avoids the need for a type test within `compare_const_vals`.
The code is clearer and simpler without it. Note that the `a == b` early return at the top of the function means the `a == b` test at the end of the function could never succeed.
It's now only used in no-longer-interesting assertion.
Because these evaluations can never fail.
ca5e990
to
32b011a
Compare
This commit removes the `a == b` early return, which isn't useful in practice, and replaces it with one that helps matches with many ranges, including char ranges.
32b011a
to
bdbf9b2
Compare
I have updated the final commit as requested. |
@bors r+ |
📌 Commit bdbf9b2 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (cacc75c): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
Various optimizations and cleanups aimed at improving compilation of
unicode-normalization
, which is notable for having several very largematch
es with many char ranges.Best reviewed one commit at a time.
r? @oli-obk