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

core/num: Speed up from_str_radix() method #55973

Closed
wants to merge 2 commits into from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Nov 15, 2018

This commit changes the from_str() implementation to use the from_str_radix() method instead of using the from_str_radix() function directly.

I did not expect any performance changes from this, but running the new benchmarks (see first commit) shows that it seems to speed up the from_str_radix() methods, while not affecting the from_str() performance itself. The i16 benchmarks seem to slightly regress for some reason, though that could also be related to them running first and test::Bencher apparently not using a warmup period (unlike e.g. criterion.rs).

Benchmark Results

 name                              control ns/iter  variable ns/iter  diff ns/iter   diff %  speedup
 num::bench_i16_from_str           33,408           33,646                     238    0.71%   x 0.99
 num::bench_i16_from_str_radix_10  31,119           32,768                   1,649    5.30%   x 0.95
 num::bench_i16_from_str_radix_16  32,198           35,726                   3,528   10.96%   x 0.90
 num::bench_i16_from_str_radix_2   23,319           21,794                  -1,525   -6.54%   x 1.07
 num::bench_i16_from_str_radix_36  31,440           34,217                   2,777    8.83%   x 0.92
 num::bench_i32_from_str           33,248           33,235                     -13   -0.04%   x 1.00
 num::bench_i32_from_str_radix_10  33,138           31,791                  -1,347   -4.06%   x 1.04
 num::bench_i32_from_str_radix_16  37,350           35,867                  -1,483   -3.97%   x 1.04
 num::bench_i32_from_str_radix_2   22,602           21,269                  -1,333   -5.90%   x 1.06
 num::bench_i32_from_str_radix_36  36,946           35,550                  -1,396   -3.78%   x 1.04
 num::bench_i64_from_str           40,238           41,167                     929    2.31%   x 0.98
 num::bench_i64_from_str_radix_10  39,251           35,293                  -3,958  -10.08%   x 1.11
 num::bench_i64_from_str_radix_16  40,549           36,417                  -4,132  -10.19%   x 1.11
 num::bench_i64_from_str_radix_2   25,645           22,119                  -3,526  -13.75%   x 1.16
 num::bench_i64_from_str_radix_36  41,253           37,102                  -4,151  -10.06%   x 1.11
 num::bench_i8_from_str            35,448           33,067                  -2,381   -6.72%   x 1.07
 num::bench_i8_from_str_radix_10   33,548           31,128                  -2,420   -7.21%   x 1.08
 num::bench_i8_from_str_radix_16   34,571           32,268                  -2,303   -6.66%   x 1.07
 num::bench_i8_from_str_radix_2    27,035           25,228                  -1,807   -6.68%   x 1.07
 num::bench_i8_from_str_radix_36   32,983           30,924                  -2,059   -6.24%   x 1.07
 num::bench_u16_from_str           24,972           24,995                      23    0.09%   x 1.00
 num::bench_u16_from_str_radix_10  30,680           28,935                  -1,745   -5.69%   x 1.06
 num::bench_u16_from_str_radix_16  33,354           31,709                  -1,645   -4.93%   x 1.05
 num::bench_u16_from_str_radix_2   23,712           22,231                  -1,481   -6.25%   x 1.07
 num::bench_u16_from_str_radix_36  32,593           30,699                  -1,894   -5.81%   x 1.06
 num::bench_u32_from_str           26,377           26,373                      -4   -0.02%   x 1.00
 num::bench_u32_from_str_radix_10  36,649           36,282                    -367   -1.00%   x 1.01
 num::bench_u32_from_str_radix_16  41,986           40,659                  -1,327   -3.16%   x 1.03
 num::bench_u32_from_str_radix_2   25,700           21,984                  -3,716  -14.46%   x 1.17
 num::bench_u32_from_str_radix_36  40,881           36,783                  -4,098  -10.02%   x 1.11
 num::bench_u64_from_str           23,685           23,756                      71    0.30%   x 1.00
 num::bench_u64_from_str_radix_10  37,419           29,654                  -7,765  -20.75%   x 1.26
 num::bench_u64_from_str_radix_16  41,441           32,673                  -8,768  -21.16%   x 1.27
 num::bench_u64_from_str_radix_2   26,958           23,166                  -3,792  -14.07%   x 1.16
 num::bench_u64_from_str_radix_36  43,619           34,687                  -8,932  -20.48%   x 1.26
 num::bench_u8_from_str            24,187           24,219                      32    0.13%   x 1.00
 num::bench_u8_from_str_radix_10   36,495           30,844                  -5,651  -15.48%   x 1.18
 num::bench_u8_from_str_radix_16   35,309           31,433                  -3,876  -10.98%   x 1.12
 num::bench_u8_from_str_radix_2    26,431           24,125                  -2,306   -8.72%   x 1.10
 num::bench_u8_from_str_radix_36   36,213           31,624                  -4,589  -12.67%   x 1.15

(https://github.com/BurntSushi/cargo-benchcmp was used to create the above result table)

Note that this does not include the changes from #55932 yet.

This commit changes the `from_str()` implementation to use the `from_str_radix()` *method* instead of using the `from_str_radix()` *function* directly.

I did not expect any performance changes from this, but running the new benchmarks shows that it seems to speed up the `from_str_radix()` benchmarks, while not affecting the `from_str()` performance. The `i16` benchmarks seem to slightly regress for some reason, though that could also be related to them running first and `test::Bencher` not using a warmup period (unlike criterion.rs).

### Benchmark Results

```
 name                              control.txt ns/iter  variable.txt ns/iter  diff ns/iter   diff %  speedup
 num::bench_i16_from_str           33,408               33,646                         238    0.71%   x 0.99
 num::bench_i16_from_str_radix_10  31,119               32,768                       1,649    5.30%   x 0.95
 num::bench_i16_from_str_radix_16  32,198               35,726                       3,528   10.96%   x 0.90
 num::bench_i16_from_str_radix_2   23,319               21,794                      -1,525   -6.54%   x 1.07
 num::bench_i16_from_str_radix_36  31,440               34,217                       2,777    8.83%   x 0.92
 num::bench_i32_from_str           33,248               33,235                         -13   -0.04%   x 1.00
 num::bench_i32_from_str_radix_10  33,138               31,791                      -1,347   -4.06%   x 1.04
 num::bench_i32_from_str_radix_16  37,350               35,867                      -1,483   -3.97%   x 1.04
 num::bench_i32_from_str_radix_2   22,602               21,269                      -1,333   -5.90%   x 1.06
 num::bench_i32_from_str_radix_36  36,946               35,550                      -1,396   -3.78%   x 1.04
 num::bench_i64_from_str           40,238               41,167                         929    2.31%   x 0.98
 num::bench_i64_from_str_radix_10  39,251               35,293                      -3,958  -10.08%   x 1.11
 num::bench_i64_from_str_radix_16  40,549               36,417                      -4,132  -10.19%   x 1.11
 num::bench_i64_from_str_radix_2   25,645               22,119                      -3,526  -13.75%   x 1.16
 num::bench_i64_from_str_radix_36  41,253               37,102                      -4,151  -10.06%   x 1.11
 num::bench_i8_from_str            35,448               33,067                      -2,381   -6.72%   x 1.07
 num::bench_i8_from_str_radix_10   33,548               31,128                      -2,420   -7.21%   x 1.08
 num::bench_i8_from_str_radix_16   34,571               32,268                      -2,303   -6.66%   x 1.07
 num::bench_i8_from_str_radix_2    27,035               25,228                      -1,807   -6.68%   x 1.07
 num::bench_i8_from_str_radix_36   32,983               30,924                      -2,059   -6.24%   x 1.07
 num::bench_u16_from_str           24,972               24,995                          23    0.09%   x 1.00
 num::bench_u16_from_str_radix_10  30,680               28,935                      -1,745   -5.69%   x 1.06
 num::bench_u16_from_str_radix_16  33,354               31,709                      -1,645   -4.93%   x 1.05
 num::bench_u16_from_str_radix_2   23,712               22,231                      -1,481   -6.25%   x 1.07
 num::bench_u16_from_str_radix_36  32,593               30,699                      -1,894   -5.81%   x 1.06
 num::bench_u32_from_str           26,377               26,373                          -4   -0.02%   x 1.00
 num::bench_u32_from_str_radix_10  36,649               36,282                        -367   -1.00%   x 1.01
 num::bench_u32_from_str_radix_16  41,986               40,659                      -1,327   -3.16%   x 1.03
 num::bench_u32_from_str_radix_2   25,700               21,984                      -3,716  -14.46%   x 1.17
 num::bench_u32_from_str_radix_36  40,881               36,783                      -4,098  -10.02%   x 1.11
 num::bench_u64_from_str           23,685               23,756                          71    0.30%   x 1.00
 num::bench_u64_from_str_radix_10  37,419               29,654                      -7,765  -20.75%   x 1.26
 num::bench_u64_from_str_radix_16  41,441               32,673                      -8,768  -21.16%   x 1.27
 num::bench_u64_from_str_radix_2   26,958               23,166                      -3,792  -14.07%   x 1.16
 num::bench_u64_from_str_radix_36  43,619               34,687                      -8,932  -20.48%   x 1.26
 num::bench_u8_from_str            24,187               24,219                          32    0.13%   x 1.00
 num::bench_u8_from_str_radix_10   36,495               30,844                      -5,651  -15.48%   x 1.18
 num::bench_u8_from_str_radix_16   35,309               31,433                      -3,876  -10.98%   x 1.12
 num::bench_u8_from_str_radix_2    26,431               24,125                      -2,306   -8.72%   x 1.10
 num::bench_u8_from_str_radix_36   36,213               31,624                      -4,589  -12.67%   x 1.15
```
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2018
@alexcrichton
Copy link
Member

Hm so it looks like the from_str_radix method simply calls the from_str_radix free function, so I wonder if the results here are perhaps in the noise?

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 15, 2018

so I wonder if the results here are perhaps in the noise?

I was wondering the same thing, but I ran the benchmarks (both before and after) multiple times and got consistent results 🤔

@alexcrichton
Copy link
Member

alexcrichton commented Nov 15, 2018

Perhaps the variance is large enough to subsume the results? There's no clear reason to me why this method would be faster than the previous method

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 15, 2018

There's no clear reason to me why this method would be faster than the previous method

I totally agree... 😅

Perhaps the variance is large enough to subsume the results?

but does that explain why multiple benchmarks result in roughly the same numbers on both sides? could you check on your machine if you get similar results?

@alexcrichton
Copy link
Member

Sorry I haven't had much time to reproduce this, but I can pretty confidently say that this patch shouldn't have that much impact on performance. If you want to land the benchmarks though we can!

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 20, 2018

If you want to land the benchmarks though we can!

sure, I guess that'd be good independent from the code logic change. do you want me to open a dedicated PR for that or should I modify this one?

@alexcrichton
Copy link
Member

Modifying this PR is fine!

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 21, 2018

I decided to open a dedicated PR (#56126) for the benchmarks so that I didn't have to adjust the original PR description etc. Closing this PR in favor of #56126.

@Turbo87 Turbo87 closed this Nov 21, 2018
@Turbo87 Turbo87 deleted the from_str_radix_speedup branch November 21, 2018 10:50
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 22, 2018
core/benches/num: Add `from_str/from_str_radix()` benchmarks

This was extracted from rust-lang#55973

/cc @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants