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.

Sign up for GitHub

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

Jump to bottom

Prefer match over combinators to make some Box methods inlineable #81826

Merged
merged 1 commit into from
Feb 9, 2021
Merged

Prefer match over combinators to make some Box methods inlineable #81826

merged 1 commit into from
Feb 9, 2021

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Feb 6, 2021

Hopefully this patch would make two snippets generated identical code: https://rust.godbolt.org/z/fjrj4E.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 Feb 6, 2021
@tesuji
Copy link
Contributor Author

tesuji commented Feb 6, 2021

r? @Amanieu as allocator working group member.

@rust-highfive rust-highfive assigned Amanieu and unassigned dtolnay Feb 6, 2021
@Amanieu
Copy link
Member

Amanieu commented Feb 6, 2021

Both examples on godbolt already generate identical code. #[inline] shouldn't be needed here since the function is already generic.

@tesuji
Copy link
Contributor Author

tesuji commented Feb 6, 2021 •
edited
Loading

I would expect that <alloc::boxed::Box<[bool; 2048]>>::new_zeroed_in::{closure#0}: is inlined
to make code size smaller.

Or could we change unwrap_or_else in new_zeroed_in to use match expression to get rid of that closure ?

@Amanieu
Copy link
Member

Amanieu commented Feb 6, 2021

The #[inline] is wrong here, you would want it on the closure. But in this case a match is preferable since it is faster to compile.

@tesuji tesuji changed the title Mark Box::new_zeroed_in inline Prefer match over combinators to make some Box methods inlineable Feb 6, 2021
@tesuji
Copy link
Contributor Author

tesuji commented Feb 6, 2021

Alright. I changed the code to prefer match expression over Result combinators.

@Amanieu
Copy link
Member

Amanieu commented Feb 6, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 6, 2021

📌 Commit fb4e734 has been approved by Amanieu

@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 Feb 6, 2021
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 9, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2021
@bors
Auto merge of rust-lang#81905 - Dylan-DPC:rollup-mxpz1j7, r=Dylan-DPC
f4008fe 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#72209 (Add checking for no_mangle to unsafe_code lint)
 - rust-lang#80732 (Allow Trait inheritance with cycles on associated types take 2)
 - rust-lang#81697 (Add "every" as a doc alias for "all".)
 - rust-lang#81826 (Prefer match over combinators to make some Box methods inlineable)
 - rust-lang#81834 (Resolve typedef in HashMap lldb pretty-printer only if possible)
 - rust-lang#81841 ([rustbuild] Output rustdoc-json-types docs )
 - rust-lang#81849 (Expand the docs for ops::ControlFlow a bit)
 - rust-lang#81876 (parser: Fix panic in 'const impl' recovery)
 - rust-lang#81882 (:arrow_up: rust-analyzer)
 - rust-lang#81888 (Fix pretty printer macro_rules with semicolon.)
 - rust-lang#81896 (Remove outdated comment in windows' mutex.rs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d19f375 into rust-lang:master Feb 9, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 9, 2021
@tesuji tesuji deleted the inline-box-zeros branch February 9, 2021 08:50
@tesuji
Copy link
Contributor Author

tesuji commented Feb 11, 2021

Report: Now the two snippet generate identical asm without intermediate closure: https://rust.godbolt.org/z/fjrj4E

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Amanieu Amanieu

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.
6 participants
@tesuji @rust-highfive @Amanieu @bors @dtolnay @rustbot