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 fmt::PadAdapter::wrap #87052

Merged
merged 1 commit into from
Jul 30, 2021
Merged

Optimize fmt::PadAdapter::wrap #87052

merged 1 commit into from
Jul 30, 2021

Conversation

phlopsi
Copy link
Contributor

@phlopsi phlopsi commented Jul 11, 2021

After adding the first write! usage to my project and printing the result to the console, I noticed, that my binary contains the strings "called Option::unwrap() on a None value`" and more importantly "C:\Users\Patrick Fischer.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\builders.rs", with my release build being configured as follows:

[profile.release]
panic = "abort"
codegen-units = 1
strip = "symbols" # the important bit
lto = true

I am in a no_std environment and my custom panic handler is a simple loop {}. I did not expect the above information to be preserved. I heavily suspect the edited function to be the culprit. It contains the only direct use of Option::unwrap in the entire file and I tracked the symbols in the assembly to be used from the section _ZN68_$LT$core..fmt..builders..PadAdapter$u20$as$u20$core..fmt..Write$GT$9write_str17ha1d5e5efe167202aE.

Aside from me suspecting this function to be the culprit, the replaced code performs the same operation as Option::insert, but without the unreachable_unchecked optimization Option::insert provides. Therefore, it makes sense to me to use the more optimized version, instead.

As I don't change any semantics, I hope a simple pull request suffices.

@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 @joshtriplett (or someone else) soon.

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 Jul 11, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@camelid camelid added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 29, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 30, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 30, 2021

📌 Commit 3e82dca has been approved by jyn514

@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 30, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#87052 (Optimize fmt::PadAdapter::wrap)
 - rust-lang#87522 (Fix assert in diy_float)
 - rust-lang#87553 (Fix typo in rustc_driver::version)
 - rust-lang#87554 (2229: Discr should be read when PatKind is Range)
 - rust-lang#87564 (min_type_alias_impl_trait is going to be removed in 1.56)
 - rust-lang#87574 (Update the examples in `String` and `VecDeque::retain`)
 - rust-lang#87583 (Refactor compression cache in v0 symbol mangler)
 - rust-lang#87585 (Add missing links for core::char types)
 - rust-lang#87594 (fs File get_path procfs usage for netbsd same as linux.)
 - rust-lang#87602 ([backtraces]: look for the `begin` symbol only after seeing `end`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c25b979 into rust-lang:master Jul 30, 2021
@rustbot rustbot added this to the 1.56.0 milestone Jul 30, 2021
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants