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

special case for integer log10 #86930

Merged
merged 2 commits into from
Jul 8, 2021
Merged

special case for integer log10 #86930

merged 2 commits into from
Jul 8, 2021

Conversation

tspiteri
Copy link
Contributor

@tspiteri tspiteri commented Jul 7, 2021

Now that #80918 has been merged, this PR provides a faster version of log10.

The PR also adds some tests for values close to all powers of 10.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Jul 7, 2021
@tspiteri
Copy link
Contributor Author

tspiteri commented Jul 7, 2021

Some timings of old versus new:

test u128_new ... bench:       8,793 ns/iter (+/- 509)
test u128_old ... bench:      28,489 ns/iter (+/- 892)
test u16_new  ... bench:         259 ns/iter (+/- 12)
test u16_old  ... bench:         921 ns/iter (+/- 27)
test u32_new  ... bench:         718 ns/iter (+/- 21)
test u32_old  ... bench:       1,691 ns/iter (+/- 25)
test u64_new  ... bench:         848 ns/iter (+/- 23)
test u64_old  ... bench:       4,221 ns/iter (+/- 95)
test u8_new   ... bench:         219 ns/iter (+/- 8)
test u8_old   ... bench:         371 ns/iter (+/- 6)
Benchmark details

macro_rules! bench {
    ($T:ident, $old:ident, $new:ident) => {
        // for example for u32, compute:
        //   * 1, 2, 3, ..., 255
        //   * 1 << 8, 2 << 8, 3 << 8, ..., 255 << 8
        //   * 1 << 16, 2 << 16, 3 << 16, ..., 255 << 16
        //   * 1 << 24, 2 << 24, 3 << 24, ..., 255 << 24

        #[bench]
        fn $old(bench: &mut Bencher) {
            for n in 0..(<$T>::BITS / 8) {
                bench.iter(|| {
                    for i in 1..=(u8::MAX as $T) {
                        let i = i << (n * 8);
                        black_box(old::$T(i, 10));
                    }
                });
            }
        }

        #[bench]
        fn $new(bench: &mut Bencher) {
            for n in 0..(<$T>::BITS / 8) {
                bench.iter(|| {
                    for i in 1..=(u8::MAX as $T) {
                        let i = i << (n * 8);
                        black_box(int_log10::$T(i));
                    }
                });
            }
        }
    };
}

bench! { u8, u8_old, u8_new }
bench! { u16, u16_old, u16_new }
bench! { u32, u32_old, u32_new }
bench! { u64, u64_old, u64_new }
bench! { u128, u128_old, u128_new }

@rust-log-analyzer

This comment has been minimized.

@kennytm
Copy link
Member

kennytm commented Jul 7, 2021

@bors rollup=never

@kennytm
Copy link
Member

kennytm commented Jul 7, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 7, 2021

📌 Commit ed76c11 has been approved by kennytm

@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 Jul 7, 2021
@bors
Copy link
Contributor

bors commented Jul 7, 2021

⌛ Testing commit ed76c11 with merge c014ff7e6d366543042d6b70e40d023cf27dbdf7...

@bors
Copy link
Contributor

bors commented Jul 7, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 7, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@tspiteri
Copy link
Contributor Author

tspiteri commented Jul 7, 2021

The failure seems spurious, or am I missing something?

1
} else {
0
}
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried doing a binary search instead a linear chain of inequalities? I'm not sure if it would actually be faster due to unpredictable branches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try it and it seemed to be slightly slower (just over the error thresholds), so I thought it better to leave the simpler code since there was no clear gain.

@the8472
Copy link
Member

the8472 commented Jul 7, 2021

@bors retry

@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 Jul 7, 2021
@bors
Copy link
Contributor

bors commented Jul 7, 2021

⌛ Testing commit ed76c11 with merge 8879bfb0f219936a8255549ba640b7f2f40b793d...

@bors
Copy link
Contributor

bors commented Jul 8, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 8, 2021
@JohnTitor
Copy link
Member

@bors retry

@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 Jul 8, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jul 8, 2021

⌛ Testing commit ed76c11 with merge 3f75f8508a00f5046784fa6cec436c926ab6a1b8...

@bors
Copy link
Contributor

bors commented Jul 8, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 8, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@JohnTitor
Copy link
Member

@bors retry

@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 Jul 8, 2021
@bors
Copy link
Contributor

bors commented Jul 8, 2021

⌛ Testing commit ed76c11 with merge 8b87e85...

@bors
Copy link
Contributor

bors commented Jul 8, 2021

☀️ Test successful - checks-actions
Approved by: kennytm
Pushing 8b87e85 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 8, 2021
@bors bors merged commit 8b87e85 into rust-lang:master Jul 8, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 8, 2021
@tspiteri tspiteri deleted the int_log10 branch August 19, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

8 participants