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

to_digit simplification (less jumps) #85630

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented May 24, 2021

I just realised we might be able to make use of the fact that changing case in ascii is easy to help simplify to_digit some more.

It looks a bit cleaner and it looks like it's less jumps and there's less instructions in the generated assembly:

https://godbolt.org/z/84Erh5dhz

The benchmarks don't really tell me much. Maybe a slight improvement on the var radix.

Before:

test char::methods::bench_to_digit_radix_10                     ... bench:      53,819 ns/iter (+/- 8,314)
test char::methods::bench_to_digit_radix_16                     ... bench:      57,265 ns/iter (+/- 10,730)
test char::methods::bench_to_digit_radix_2                      ... bench:      55,077 ns/iter (+/- 5,431)
test char::methods::bench_to_digit_radix_36                     ... bench:      56,549 ns/iter (+/- 3,248)
test char::methods::bench_to_digit_radix_var                    ... bench:      43,848 ns/iter (+/- 3,189)

test char::methods::bench_to_digit_radix_10                     ... bench:      51,707 ns/iter (+/- 10,946)
test char::methods::bench_to_digit_radix_16                     ... bench:      52,835 ns/iter (+/- 2,689)
test char::methods::bench_to_digit_radix_2                      ... bench:      51,012 ns/iter (+/- 2,746)
test char::methods::bench_to_digit_radix_36                     ... bench:      53,210 ns/iter (+/- 8,645)
test char::methods::bench_to_digit_radix_var                    ... bench:      40,386 ns/iter (+/- 4,711)

test char::methods::bench_to_digit_radix_10                     ... bench:      54,088 ns/iter (+/- 5,677)
test char::methods::bench_to_digit_radix_16                     ... bench:      55,972 ns/iter (+/- 17,229)
test char::methods::bench_to_digit_radix_2                      ... bench:      52,083 ns/iter (+/- 2,425)
test char::methods::bench_to_digit_radix_36                     ... bench:      54,132 ns/iter (+/- 1,548)
test char::methods::bench_to_digit_radix_var                    ... bench:      41,250 ns/iter (+/- 5,299)

After:

test char::methods::bench_to_digit_radix_10                     ... bench:      48,907 ns/iter (+/- 19,449)
test char::methods::bench_to_digit_radix_16                     ... bench:      52,673 ns/iter (+/- 8,122)
test char::methods::bench_to_digit_radix_2                      ... bench:      48,509 ns/iter (+/- 2,885)
test char::methods::bench_to_digit_radix_36                     ... bench:      50,526 ns/iter (+/- 4,610)
test char::methods::bench_to_digit_radix_var                    ... bench:      38,618 ns/iter (+/- 3,180)

test char::methods::bench_to_digit_radix_10                     ... bench:      54,202 ns/iter (+/- 6,994)
test char::methods::bench_to_digit_radix_16                     ... bench:      56,585 ns/iter (+/- 8,448)
test char::methods::bench_to_digit_radix_2                      ... bench:      50,548 ns/iter (+/- 1,674)
test char::methods::bench_to_digit_radix_36                     ... bench:      52,749 ns/iter (+/- 2,576)
test char::methods::bench_to_digit_radix_var                    ... bench:      40,215 ns/iter (+/- 3,327)

test char::methods::bench_to_digit_radix_10                     ... bench:      50,233 ns/iter (+/- 22,272)
test char::methods::bench_to_digit_radix_16                     ... bench:      50,841 ns/iter (+/- 19,981)
test char::methods::bench_to_digit_radix_2                      ... bench:      50,386 ns/iter (+/- 4,555)
test char::methods::bench_to_digit_radix_36                     ... bench:      52,369 ns/iter (+/- 2,737)
test char::methods::bench_to_digit_radix_var                    ... bench:      40,417 ns/iter (+/- 2,766)

I removed the likely as it resulted in a few less instructions. (It's not been in there long - I added it in the last to_digit iteration).

@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 May 24, 2021
@klensy
Copy link
Contributor

klensy commented May 24, 2021

There bultin diff function in godbolt: https://godbolt.org/z/jMsv6dKh8

@rust-log-analyzer

This comment has been minimized.

@gilescope
Copy link
Contributor Author

Good point. Have flicked the code around so the red / green works in the diff and optimised the to upper case:

https://godbolt.org/z/79KrbxMdY

@rust-log-analyzer

This comment has been minimized.

@nagisa
Copy link
Member

nagisa commented May 24, 2021

I don't believe this implementation is correct. E.g. it will return Some(10) for ascii : in base 11.

Some other issues: A is Some(17) in base 36, a is None in base 36, even though both should be returning a Some(10).

(Can you add cases like these as tests, please?)

@gilescope
Copy link
Contributor Author

Sorry jumped strate to the bench skipping tests - too much excitement.

@rust-log-analyzer

This comment has been minimized.

@gilescope
Copy link
Contributor Author

Ok am really happy with that: https://godbolt.org/z/n77njGz3T

@gilescope
Copy link
Contributor Author

@nagisa is this all looking good now from your perspective?

@nagisa
Copy link
Member

nagisa commented May 29, 2021

Squash the commits, please. Otherwise looks pretty nice.

@klensy
Copy link
Contributor

klensy commented May 30, 2021

Can you update benches in first message with latest version?

@nagisa
Copy link
Member

nagisa commented Jun 6, 2021

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned kennytm Jun 6, 2021
@gilescope
Copy link
Contributor Author

will do

@gilescope
Copy link
Contributor Author

Have updated the benchmarks. I'm not really happy with the benchmark definitions though I have failed to come up with an alternative that has less benchmark noise.

@gilescope gilescope force-pushed the to_digit_speedup3 branch from bcea845 to 9c3d81e Compare June 10, 2021 19:21
@gilescope
Copy link
Contributor Author

Ok @nagisa think it is good to go.

@nagisa
Copy link
Member

nagisa commented Jun 10, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jun 10, 2021

📌 Commit 9c3d81e has been approved by nagisa

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

bors commented Jun 10, 2021

⌛ Testing commit 9c3d81e with merge 46ad16b...

@klensy
Copy link
Contributor

klensy commented Jun 11, 2021

@gilescope You can use cargo-benchcmp to compare bench results in more simple way.

@bors
Copy link
Contributor

bors commented Jun 11, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 46ad16b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 11, 2021
@bors bors merged commit 46ad16b into rust-lang:master Jun 11, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 11, 2021
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