Skip to content

Conversation

kornelski
Copy link
Contributor

This sets #[inline(always)] on trivial 1-liner functions in core. The goal is to improve performance of code in debug mode at opt-level 0 and 1, when inlining doesn't happen otherwise.

@rust-highfive
Copy link
Contributor

r? @scottmcm

(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 May 11, 2021
@Mark-Simulacrum
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 11, 2021
@bors
Copy link
Collaborator

bors commented May 11, 2021

⌛ Trying commit 0f4886f89cb654ee67176b897139b88d66ee6c8f with merge 71f3cadcc5df5a85f5db16ec986d73ed5bb6b7c6...

@bors
Copy link
Collaborator

bors commented May 12, 2021

☀️ Try build successful - checks-actions
Build commit: 71f3cadcc5df5a85f5db16ec986d73ed5bb6b7c6 (71f3cadcc5df5a85f5db16ec986d73ed5bb6b7c6)

@rust-timer
Copy link
Collaborator

Queued 71f3cadcc5df5a85f5db16ec986d73ed5bb6b7c6 with parent 5c02926, future comparison URL.

@scottmcm
Copy link
Member

These two PRs feel like it might be getting too pervasive. I understand it for things like ptr::read that should behave like intrinsics in debug mode because people expect it to just be *p, but "every trivial method" feels like it should be a compiler change and not a library change -- especially since people will use core doing it to argue that the whole ecosystem should start doing it too, which I'd really rather not de-facto encourage. (Is there a @rust-lang/libs-impl I could ping? This is literally my first even PR assigned by high-five, so I might be missing some context about a different decision here...)

I see that @tmiasko and @wesleywiser have been doing perf runs lately about running the MIR inliner in more situations -- perhaps you have thoughts on whether rustc could just start inlining these kinds of things without needing annotations?

@workingjubilee
Copy link
Member

Note Mara's comment in #84560 (comment) and the fact that #[inline(always)] in that PR actually caused various regressions, in spite of it being a trivial and "obvious" change. Giving such an imperiously strong directive to LLVM is not necessarily beneficial.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (71f3cadcc5df5a85f5db16ec986d73ed5bb6b7c6): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 12, 2021
@kornelski kornelski changed the title #[inline(always)] on basic range and slice methods #[inline(always)] on basic pointer methods May 12, 2021
@kornelski
Copy link
Contributor Author

@scottmcm ok, I've narrowed this one down to just pointer methods.

@kornelski kornelski closed this May 12, 2021
@kornelski kornelski deleted the rangeinline branch May 12, 2021 09:19
@kornelski kornelski restored the rangeinline branch May 12, 2021 09:22
@kornelski
Copy link
Contributor Author

Oops, I've accidentally closed this one. Reopened as #85218

bors added a commit to rust-lang-ci/rust that referenced this pull request May 12, 2021
#[inline(always)] on basic pointer methods

Retryng rust-lang#85201 with only inlining pointer methods. The goal is to make pointers behave just like pointers in O0, mainly to reduce overhead in debug builds.

cc `@scottmcm`
@kornelski kornelski deleted the rangeinline branch May 13, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants