Skip to content
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

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

nhwn
Copy link
Contributor

@nhwn nhwn commented Feb 18, 2021

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.

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 18, 2021
@rust-log-analyzer

This comment has been minimized.

@leonardo-m
Copy link

An alternative version:

let mut num_digits = 0;
loop {
    num_digits += 1;
    n /= 10;
    if n == 0 { break; }
}
num_digits

@nhwn
Copy link
Contributor Author

nhwn commented Feb 18, 2021

An alternative version:

let mut num_digits = 0;
loop {
    num_digits += 1;
    n /= 10;
    if n == 0 { break; }
}
num_digits

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);
Copy link
Member

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an explanatory comment.

@varkor
Copy link
Member

varkor commented Feb 18, 2021

Could you squash the commits? Thanks.

@varkor
Copy link
Member

varkor commented Feb 18, 2021

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 18, 2021

📌 Commit 2cd7a5148ce00e403f030e1b9437bd99e93cb08e has been approved by varkor

@bors bors 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 Feb 18, 2021
@rust-log-analyzer

This comment has been minimized.

@nhwn
Copy link
Contributor Author

nhwn commented Feb 18, 2021

It seems my tidy hook didn't auto-run when I rebased, so a pesky space slipped through in the comment. Sorry about that.

@varkor
Copy link
Member

varkor commented Feb 18, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 18, 2021

📌 Commit 8a5c568 has been approved by varkor

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 18, 2021
…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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2021
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
@bors bors merged commit 555db2d into rust-lang:master Feb 18, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 18, 2021
@nhwn nhwn deleted the optimize-counting-digits branch February 18, 2021 22:39
@matthieu-m
Copy link
Contributor

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):

fn count_digits(i: u32) -> u32 {
    if i < 10 {
        return 1;
    }
    if i < 100 {
        return 2;
    }

    match 32 - i.leading_zeros() {
        0 | 1 | 2 | 3 | 4 | 5 | 6
            //  100 takes 7 bits, and anything < 100 is already handled.
            => unsafe { hint::unreachable_unchecked() },
        7 | 8 | 9 | 10
            => 3 + if i < 1_000 { 0 } else { 1 },
        11 | 12 | 13 | 14
            => 4 + if i < 10_000 { 0 } else { 1 },
        15 | 16 | 17
            => 5 + if i < 100_000 { 0 } else { 1 },
        18 | 19 | 20
            => 6 + if i < 1_000_000 { 0 } else { 1 },
        21 | 22 | 23 | 24
            => 7 + if i < 10_000_000 { 0 } else { 1 },
        25 | 26 | 27
            => 8 + if i < 100_000_000 { 0 } else { 1 },
        28 | 29 | 30
            => 9 + if i < 1_000_000_000 { 0 } else { 1 },
        31 | 32 => 10,
        //  There are not more than 32 bits in a 32 bits integer
        _ => unsafe { hint::unreachable_unchecked() },
    }
}

It may be a useful addition to the standard library.

@toothbrush7777777
Copy link

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.
[…]

Basically, you want ceil(log10(n)).

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 2, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants