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

Consider inlining Poll::map, Poll::map_ok and Poll::map_err #101080

Closed
EFanZh opened this issue Aug 27, 2022 · 7 comments · Fixed by #107987
Closed

Consider inlining Poll::map, Poll::map_ok and Poll::map_err #101080

EFanZh opened this issue Aug 27, 2022 · 7 comments · Fixed by #107987

Comments

@EFanZh
Copy link
Contributor

EFanZh commented Aug 27, 2022

I noticed that in one of my project, inlining Poll::map manually reduces some binary size, then I noticed Poll::map is not marked #[inline]. Since both Option::map and Result::map are marked #[inline], I wonder whether this is an oversight. I failed to reproduce the binary size reduction effect with an example code, but maybe the reason behind inlining Option::map and Result::map also applies to Poll::map? How about Poll::map_ok and Poll::map_err?

@saethlin
Copy link
Member

That's surprising. Can you share your project?

@EFanZh
Copy link
Contributor Author

EFanZh commented Aug 27, 2022

@saethlin Unfortunately, it is a private project, I cannot share it.

@saethlin
Copy link
Member

saethlin commented Aug 27, 2022

Ah okay. I find it suspicious that these functions aren't inlining already. Did you try building your project against a modified standard library with #[inline] added to those functions?

Do you still get a code size improvement from manual inlining if you compile with codegen-units = 1 and/or lto = "fat" in your release profile? The functions in question ought to already be eligible for inlining, but by default the release profile has multiple codegen units per crate, which can result in surprising failures to inline things.

How big are these functions once they are compiled in your crate? The third-party tool cargo-bloat can help out here, can you share the output of

cargo bloat --release -n 1000000 | grep ::map

(of course trimming that down a bit if there are type names in there you don't want to fully disclose)
If for whatever reason these functions end up looking big to LLVM after other things are inlined into them, that may explain what's going on here, and your manual inlining has effectively changed in what order LLVM considers inlining things.

@EFanZh
Copy link
Contributor Author

EFanZh commented Sep 13, 2022

@saethlin I have done some tests on my project, using nm to check for symbols:

  • Using lto=thin, Poll::map does not get inlined;
  • Using lto=fat, Poll::map gets inlined;
  • Using lto=thin and build-std, add #[inline] to standard library source code, Poll::map gets inlined;

All configurations above are using codgen-units=1 and opt-level=z.

nm reports that each Poll::map is about 68 to 96 bytes.

@saethlin
Copy link
Member

What about lto=thin and #[inline] added to the standard library code, but without using -Zbuild-std?

If that configuration gets these functions inlined I think this is worth doing because it unlocks additional useful optimizations for users, but if the optimization is gated behind -Zbuild-std it's a bit less certain if this is a win.

@nikic Do you or any other LLVM folks have insight into why #[inline] would be impactful here? Are we relying on the hint part of its behavior?

@EFanZh
Copy link
Contributor Author

EFanZh commented Sep 14, 2022

@saethlin: It seems that changing standard library source code only works with build-std. Do I need to compile a customized version of Rust in order to test it?

@saethlin
Copy link
Member

That's what I do. It's a very slow workflow.

compiler-errors added a commit to compiler-errors/rust that referenced this issue Feb 26, 2023
…-Simulacrum

Inline `Poll` methods

With `opt-level="z"`, the `Poll::map*` methods are sometimes not inlined (see <https://godbolt.org/z/ca5ajKTEK>). This PR adds `#[inline]` to these methods. I have a project that can benefit from this change, but do we want to enable this behavior universally?

Fixes rust-lang#101080.
@bors bors closed this as completed in 31f858d Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants