-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Optimize counting digits in line numbers during error reporting #82248
Conversation
r? @varkor (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
An alternative version:
|
That's a much better implementation than the original one, thanks! |
@@ -1713,7 +1713,15 @@ impl EmitterWriter { | |||
let max_line_num_len = if self.ui_testing { | |||
ANONYMIZED_LINE_NUM.len() | |||
} else { | |||
self.get_max_line_num(span, children).to_string().len() | |||
let mut n = self.get_max_line_num(span, children); |
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.
Could you add a comment explaining why we calculate the number of digits this way? (To make sure someone doesn't revert this change without realising it's intentional, later.)
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.
Added an explanatory comment.
Could you squash the commits? Thanks. |
4eb41e5
to
2cd7a51
Compare
Thanks! @bors r+ rollup |
📌 Commit 2cd7a5148ce00e403f030e1b9437bd99e93cb08e has been approved by |
This comment has been minimized.
This comment has been minimized.
2cd7a51
to
8a5c568
Compare
It seems my |
@bors r+ rollup |
📌 Commit 8a5c568 has been approved by |
…rkor Optimize counting digits in line numbers during error reporting Replaces `.to_string().len()` with simple loop and integer division, which avoids an unnecessary allocation. Although I couldn't figure out how to directly profile `rustc`'s error reporting, I ran a microbenchmark on my machine (2.9 GHz Dual-Core Intel Core i5) on the two strategies for `0..100_000`, and the results seem promising: ``` test to_string_len ... bench: 12,124,792 ns/iter (+/- 700,652) test while_loop ... bench: 30,333 ns/iter (+/- 562) ``` The x86_64 disassembly reduces integer division to a multiplication + shift, so I don't think there's any problems with using integer division. For more (micro)optimization, it would be nice if we could avoid the initial check to see if the line number is nonzero, but I don't think `self.get_max_line_num(span, children)` _guarantees_ a nonzero line number.
Rollup of 10 pull requests Successful merges: - rust-lang#81546 ([libtest] Run the test synchronously when hitting thread limit) - rust-lang#82066 (Ensure valid TraitRefs are created for GATs) - rust-lang#82112 (const_generics: Dont evaluate array length const when handling yet another error ) - rust-lang#82194 (In some limited cases, suggest `where` bounds for non-type params) - rust-lang#82215 (Replace if-let and while-let with `if let` and `while let`) - rust-lang#82218 (Make sure pdbs are copied along with exe and dlls when bootstrapping) - rust-lang#82236 (avoid converting types into themselves (clippy::useless_conversion)) - rust-lang#82246 (Add long explanation for E0549) - rust-lang#82248 (Optimize counting digits in line numbers during error reporting) - rust-lang#82256 (Print -Ztime-passes (and misc stats/logs) on stderr, not stdout.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
I actually just realized there's doesn't appear to be a fast way to compute the number of (decimal) digits that an integer would take. I find it surprising, as I would have expected integer formatting to make use of this. For reference, a simple version is to switch on the number of bits (playground):
It may be a useful addition to the standard library. |
Basically, you want |
Optimize counting digits in line numbers during error reporting further This one-ups rust-lang#82248 by switching the strategy: Instead of dividing the value by 10 repeatedly, we compare with a limit that we multiply by 10 repeatedly. In my benchmarks, this took between 50% and 25% of the original time. The reasons for being faster are: 1. While LLVM is able to replace a division by constant with a multiply + shift, a plain multiplication is still faster. However, this doesn't even factor, because 2. Multiplication, unlike division, is const. We also use a simple for-loop instead of a more complex loop + break, which allows 3. rustc to const-fold the whole loop, and indeed the assembly output simply shows a series of comparisons.
Replaces
.to_string().len()
with simple loop and integer division, which avoids an unnecessary allocation.Although I couldn't figure out how to directly profile
rustc
's error reporting, I ran a microbenchmark on my machine (2.9 GHz Dual-Core Intel Core i5) on the two strategies for0..100_000
, and the results seem promising:The x86_64 disassembly reduces integer division to a multiplication + shift, so I don't think there's any problems with using integer division.
For more (micro)optimization, it would be nice if we could avoid the initial check to see if the line number is nonzero, but I don't think
self.get_max_line_num(span, children)
guarantees a nonzero line number.