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

AsRef/AsMut implementations aren't inlined #58867

Closed
WildCryptoFox opened this issue Mar 2, 2019 · 11 comments
Closed

AsRef/AsMut implementations aren't inlined #58867

WildCryptoFox opened this issue Mar 2, 2019 · 11 comments
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@WildCryptoFox
Copy link
Contributor

WildCryptoFox commented Mar 2, 2019

Many AsRef and AsMut implementations in core do not have #[inline] and yet these tiny no-op functions are called from the assembly.

The following assembly maps to AsMut<[T]> for [T]

Any quick search for as_ref or as_mut in convert.rs shows many instances, some with but many without #[inline].

_ZN63_$LT$$RF$mut$u20$T$u20$as$u20$core..convert..AsMut$LT$U$GT$$GT$6as_mut17h47b72d0f8c3b94ffE:
     .cfi_startproc
     movq    (%rdi), %rax
     movq    8(%rdi), %rdx
     retq

Edit: Default::default is another example where more aggressive inlining should be used

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Mar 2, 2019

You just need #[inline]. The LLVM hint wouldn't even be needed at all - #[inline] puts the function into all codegen units that reference it, that's all you typically need for small functions to be inlined.

You almost never want #[inline(always)], since it's even respected in debug mode, where it just needlessly impacts compile times.

@WildCryptoFox
Copy link
Contributor Author

Thanks @jonas-schievink for the correction.

@sfackler sfackler added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Mar 2, 2019
@Olavhaasie
Copy link

I would like to do this, If no one else has taken it yet

@sfackler sfackler removed the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Mar 3, 2019
@sfackler
Copy link
Member

sfackler commented Mar 3, 2019

LLVM should normally be inlining in release builds these unless you're using multiple codegen units without LTO.

@WildCryptoFox
Copy link
Contributor Author

WildCryptoFox commented Mar 3, 2019

@sfackler I haven't tweaked the config for codegen units nor LTO. So whatever the default is.

Edit: Why was the E-easy tag removed? Isn't this just a matter of adding #[inline] or are you saying most instances where we have #[inline] should be replaced with trusting llvm to do its part and fixing whatever relevant that isn't already triggering it in this case?

@Centril Centril added the I-slow Issue: Problems and improvements with respect to performance of generated code. label Mar 3, 2019
@sfackler
Copy link
Member

sfackler commented Mar 3, 2019

I remembered that we generally don't want to add #[inline] on functions that are already eligible for inlining: #52704.

@WildCryptoFox
Copy link
Contributor Author

Thanks @sfackler. This comment suggests using ThinLTO in release builds incremental builds disabled but obviously that isn't desired as incremental building is very desirable for rapid testing (especially in my case where I need to read the optimized assembly).

@sfackler
Copy link
Member

sfackler commented Mar 3, 2019

Incremental ThinLTO works now IIRC.

@WildCryptoFox
Copy link
Contributor Author

Cool. And enabled by default on at least the latest nightly. Should this be closed?

@lcnr
Copy link
Contributor

lcnr commented Apr 26, 2020

This seems to be fixed.

See https://rust.godbolt.org/z/j9Ts8P

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 26, 2020
check that `AsRef` and `AsMut` are inlined

Adds a regression test for rust-lang#58867

r? @Dylan-DPC
@Dylan-DPC-zz
Copy link

Closing this as it is covered by #71576

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

No branches or pull requests

7 participants