-
Notifications
You must be signed in to change notification settings - Fork 13.8k
fmt of non-decimal radix untangled #143730
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
Conversation
2281143 to
d593c6f
Compare
This comment has been minimized.
This comment has been minimized.
d593c6f to
f0f1c53
Compare
This comment has been minimized.
This comment has been minimized.
f0f1c53 to
76b8c3a
Compare
☔ The latest upstream changes (presumably #144044) made this pull request unmergeable. Please resolve the merge conflicts.
76b8c3a to
110d35e
Compare
Can you either have a look or reassign @tgross35? Got a followup pending on this one...
I'll reassign for now, but if nobody beats me to it I'll take a look on Monday
r? libs
(leaving myself assigned so it stays on my list)
By the way; if you are interested, we could use some int (and float) formatting and parsing benchmarks at https://github.com/rust-lang/rustc-perf/blob/2120e3b7b8996e96858b88edefea371679a3d415/collector/runtime-benchmarks/fmt/src/main.rs (I just learned that is possible), which would mean they get run as part of our pretty extensive perf infra rather than you needing to post the results of local runs.
Interesting indeed @tgross35. The specialized benchmarks we have at the moment (such as write_u8_min and write_u64_max) won't scale. Think about how many benchmarks we need for fmt::LowerExp with edge cases on alignment and precision. I am curious to hear what you think of the _10ints_ concept in this patch. The idea is to have a balanced mix of common cases.
110d35e to
2c6210d
Compare
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.
Few notes, haven't looked at patches 3 & 4 yet
2c6210d to
6e3b5ee
Compare
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.
Guess it's still on me, but this looks pretty good! Just a few small things
6e3b5ee to
c424403
Compare
This comment has been minimized.
This comment has been minimized.
* correct buffer size * no trait abstraction * similar to decimal
c424403 to
d7da6f7
Compare
This comment has been minimized.
This comment has been minimized.
0291e78 to
ca1d99b
Compare
@bors2 try jobs=x86_64-gnu-llvm-19-3
r=me with that passing
This comment has been minimized.
This comment has been minimized.
fmt of non-decimal radix untangled try-job: x86_64-gnu-llvm-19-3
💔 Test for 59e4e19 failed: CI. Failed jobs:
try - x86_64-gnu-llvm-19-3(web logs, extended logs)
This comment has been minimized.
This comment has been minimized.
Huh, does it get &T sometimes and just T others?
You can check that config locally with rust.std-features = ["optimize_for_size"] in your bootstrap.toml
ca1d99b to
1f77424
Compare
This comment was marked as spam.
This comment was marked as spam.
You can check that config locally with
rust.std-features = ["optimize_for_size"]in your bootstrap.toml
Apparently such changes don't always get picked up by ./x test. I will dig in deeper into why at some point if time permits.
@bors2 try jobs=x86_64-gnu-llvm-19-3
This comment has been minimized.
This comment has been minimized.
fmt of non-decimal radix untangled try-job: x86_64-gnu-llvm-19-3
Thanks for the fix!
@bors r+
Apparently such changes don't always get picked up by
./x test. I will dig in deeper into why at some point if time permits.
Maybe just bring this up the #t-infra/bootstrap Zulip, somebody may know a better idea of why
@bors rollup=maybe
...oss35 fmt of non-decimal radix untangled Have the implementation match its decimal counterpart. * Digit table instead of conversion functions * Correct buffer size per radix * Elimination of dead code for negative * No trait abstraction for integers #### Original Performance ``` fmt::write_10ints_bin 393.03ns/iter +/- 1.41 fmt::write_10ints_hex 316.84ns/iter +/- 1.49 fmt::write_10ints_oct 327.16ns/iter +/- 0.46 ``` #### Patched Performance ``` fmt::write_10ints_bin 392.31ns/iter +/- 3.05 fmt::write_10ints_hex 302.41ns/iter +/- 5.48 fmt::write_10ints_oct 322.01ns/iter +/- 3.82 ``` r? tgross35
Rollup of 15 pull requests Successful merges: - #139345 (Extend `QueryStability` to handle `IntoIterator` implementations) - #140740 (Add `-Zindirect-branch-cs-prefix`) - #142079 (nll-relate: improve hr opaque types support) - #142938 (implement std::fs::set_permissions_nofollow on unix) - #143730 (fmt of non-decimal radix untangled) - #144767 (Correct some grammar in integer documentation) - #144906 (Require approval from t-infra instead of t-release on tier bumps) - #144983 (Rehome 37 `tests/ui/issues/` tests to other subdirectories under `tests/ui/`) - #145025 (run spellcheck as a tidy extra check in ci) - #145099 (rustc_target: Add the `32s` target feature for LoongArch) - #145166 (suggest using `pub(crate)` for E0364) - #145255 (dec2flt: Provide more valid inputs examples) - #145306 (Add tracing to various miscellaneous functions) - #145336 (Hide docs for `core::unicode`) - #145585 (Miri: fix handling of in-place argument and return place handling) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #143730 - pascaldekloe:fmt-radix-trim, r=tgross35 fmt of non-decimal radix untangled Have the implementation match its decimal counterpart. * Digit table instead of conversion functions * Correct buffer size per radix * Elimination of dead code for negative * No trait abstraction for integers #### Original Performance ``` fmt::write_10ints_bin 393.03ns/iter +/- 1.41 fmt::write_10ints_hex 316.84ns/iter +/- 1.49 fmt::write_10ints_oct 327.16ns/iter +/- 0.46 ``` #### Patched Performance ``` fmt::write_10ints_bin 392.31ns/iter +/- 3.05 fmt::write_10ints_hex 302.41ns/iter +/- 5.48 fmt::write_10ints_oct 322.01ns/iter +/- 3.82 ``` r? tgross35
Rollup of 15 pull requests Successful merges: - rust-lang#139345 (Extend `QueryStability` to handle `IntoIterator` implementations) - rust-lang#140740 (Add `-Zindirect-branch-cs-prefix`) - rust-lang#142079 (nll-relate: improve hr opaque types support) - rust-lang#142938 (implement std::fs::set_permissions_nofollow on unix) - rust-lang#143730 (fmt of non-decimal radix untangled) - rust-lang#144767 (Correct some grammar in integer documentation) - rust-lang#144906 (Require approval from t-infra instead of t-release on tier bumps) - rust-lang#144983 (Rehome 37 `tests/ui/issues/` tests to other subdirectories under `tests/ui/`) - rust-lang#145025 (run spellcheck as a tidy extra check in ci) - rust-lang#145099 (rustc_target: Add the `32s` target feature for LoongArch) - rust-lang#145166 (suggest using `pub(crate)` for E0364) - rust-lang#145255 (dec2flt: Provide more valid inputs examples) - rust-lang#145306 (Add tracing to various miscellaneous functions) - rust-lang#145336 (Hide docs for `core::unicode`) - rust-lang#145585 (Miri: fix handling of in-place argument and return place handling) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 15 pull requests Successful merges: - rust-lang/rust#139345 (Extend `QueryStability` to handle `IntoIterator` implementations) - rust-lang/rust#140740 (Add `-Zindirect-branch-cs-prefix`) - rust-lang/rust#142079 (nll-relate: improve hr opaque types support) - rust-lang/rust#142938 (implement std::fs::set_permissions_nofollow on unix) - rust-lang/rust#143730 (fmt of non-decimal radix untangled) - rust-lang/rust#144767 (Correct some grammar in integer documentation) - rust-lang/rust#144906 (Require approval from t-infra instead of t-release on tier bumps) - rust-lang/rust#144983 (Rehome 37 `tests/ui/issues/` tests to other subdirectories under `tests/ui/`) - rust-lang/rust#145025 (run spellcheck as a tidy extra check in ci) - rust-lang/rust#145099 (rustc_target: Add the `32s` target feature for LoongArch) - rust-lang/rust#145166 (suggest using `pub(crate)` for E0364) - rust-lang/rust#145255 (dec2flt: Provide more valid inputs examples) - rust-lang/rust#145306 (Add tracing to various miscellaneous functions) - rust-lang/rust#145336 (Hide docs for `core::unicode`) - rust-lang/rust#145585 (Miri: fix handling of in-place argument and return place handling) r? `@ghost` `@rustbot` modify labels: rollup
...oss35 fmt of non-decimal radix untangled Have the implementation match its decimal counterpart. * Digit table instead of conversion functions * Correct buffer size per radix * Elimination of dead code for negative * No trait abstraction for integers #### Original Performance ``` fmt::write_10ints_bin 393.03ns/iter +/- 1.41 fmt::write_10ints_hex 316.84ns/iter +/- 1.49 fmt::write_10ints_oct 327.16ns/iter +/- 0.46 ``` #### Patched Performance ``` fmt::write_10ints_bin 392.31ns/iter +/- 3.05 fmt::write_10ints_hex 302.41ns/iter +/- 5.48 fmt::write_10ints_oct 322.01ns/iter +/- 3.82 ``` r? tgross35
Have the implementation match its decimal counterpart.
Original Performance
Patched Performance
r? tgross35