Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
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
Contributor

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
Collaborator

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

# for free to join this conversation on GitHub. Already have an account? # 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