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

Replace #[inline(always)] by #[inline] due to the later being conside… #141

Merged
merged 1 commit into from
Sep 20, 2017

Conversation

therealprof
Copy link
Contributor

…red harmful

Signed-off-by: Daniel Egger daniel@eggers-club.de

…red harmful

Signed-off-by: Daniel Egger <daniel@eggers-club.de>
@japaric
Copy link
Member

japaric commented Sep 19, 2017

("being considered harmful" is not a very descriptive commit message / PR title, IMO.)

For posteriority, could you please add a comment (or a link) with the rationale about the change to this PR?

@homunkulus r+

@homunkulus
Copy link
Contributor

📌 Commit 73cd404 has been approved by japaric

@homunkulus
Copy link
Contributor

⌛ Testing commit 73cd404 with merge 895722b...

japaric pushed a commit that referenced this pull request Sep 20, 2017
Replace #[inline(always)] by #[inline] due to the later being conside…

…red harmful

Signed-off-by: Daniel Egger <daniel@eggers-club.de>
@therealprof
Copy link
Contributor Author

Well, according to https://rust-lang-nursery.github.io/rust-clippy/master/index.html#inline_always it is considered harmful: It increases compile time and may increase size as well as decrease performance. I'm afraid I can't come up with a better label than "considered harmful".

@homunkulus
Copy link
Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: japaric
Pushing 895722b to master...

@homunkulus homunkulus merged commit 73cd404 into rust-embedded:master Sep 20, 2017
@japaric japaric mentioned this pull request Sep 20, 2017
@therealprof therealprof deleted the no_always_inline branch September 23, 2017 21:54
therealprof added a commit that referenced this pull request Jul 20, 2019
Turns out this @therealprof dude was wrong in blindly following general
consensus that #[inline(always)] was a bad idea. Revisiting the topic
showed that just #[inline] is not enough to get even very trivial
functions inlined in dev mode which causes a ton of bloat and a lot of
debugging fun due to many emitted extra functions without any value for
the developer.

Usual binary reductions by this change are in the area of 10-15% which
is a lot given that even a simply blinky on Cortex-M0 is several kB
already.

E.g. before:

0.5% 100.0% 4.0KiB                .text section size, the file size is 734.1KiB

after:

0.5% 100.0% 3.7KiB                .text section size, the file size is 714.5KiB

Signed-off-by: Daniel Egger <daniel@eggers-club.de>
@therealprof therealprof mentioned this pull request Jul 20, 2019
bors bot added a commit that referenced this pull request Jul 21, 2019
324: improve dev builds r=ryankurte a=therealprof

This PR basically reverts #141 and plays with inline markers for some large dev build binary size gains

Co-authored-by: Daniel Egger <daniel@eggers-club.de>
therealprof added a commit that referenced this pull request Jul 21, 2019
Turns out this @therealprof dude was wrong in blindly following general
consensus that #[inline(always)] was a bad idea. Revisiting the topic
showed that just #[inline] is not enough to get even very trivial
functions inlined in dev mode which causes a ton of bloat and a lot of
debugging fun due to many emitted extra functions without any value for
the developer.

Usual binary reductions by this change are in the area of 10-15% which
is a lot given that even a simply blinky on Cortex-M0 is several kB
already.

E.g. before:

0.5% 100.0% 4.0KiB                .text section size, the file size is 734.1KiB

after:

0.5% 100.0% 3.7KiB                .text section size, the file size is 714.5KiB

Signed-off-by: Daniel Egger <daniel@eggers-club.de>
bors bot added a commit that referenced this pull request Jul 21, 2019
324: improve dev builds r=ryankurte a=therealprof

This PR basically reverts #141 and plays with inline markers for some large dev build binary size gains

Co-authored-by: Daniel Egger <daniel@eggers-club.de>
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 this pull request may close these issues.

3 participants