fix various uses of inline

arvidn commented at 1:55 am on September 6, 2018: none

This is C++, inline functions in a headers do not need to be static. Making it so is a pessimization as it will generate functions with internal linkage in every translation unit including the header. (Note that inline in C is different from inline in C++).

Furthermore, a static function in a cpp file already has internal linkage, making it inline has no effect.

Templates already have an inline-like property of being allowed to be defined multiple times, and having the linker deal with it. Making them inline is also redundant.

This change does 3 things:

removes static from all inlined functions in header files
removes inline from all static inline functions in .cpp files
removes inline from templates

give inlined functions external linkage in header files, to avoid duplicate definitions. In cpp files, don't make functions with internal linkage inline, since it's unnecessary in that case d91b635058

templates are always inline, saying it is redundant ee258a03b4

fanquake added the label Refactoring on Sep 6, 2018

ken2812221 commented at 2:05 am on September 6, 2018: contributor

Furthermore, a static function in a cpp file already has internal linkage, making it inline has no effect.

Is there any document that can prove this? I don’t think they act different when they are in .cpp and .h files

in src/crypto/chacha20.cpp:13 in ee258a03b4

 9@@ -10,7 +10,7 @@
10 
11 #include <string.h>
12 
13-constexpr static inline uint32_t rotl32(uint32_t v, int c) { return (v << c) | (v >> (32 - c)); }
14+constexpr static uint32_t rotl32(uint32_t v, int c) { return (v << c) | (v >> (32 - c)); }

ken2812221 commented at 2:20 am on September 6, 2018:

This is constexpr , can this drop both inlin and static?

arvidn commented at 4:57 am on September 6, 2018:

no, static still gives it internal linkage.

DrahtBot commented at 3:15 am on September 6, 2018: member

#14209 (logging: Replace LogPrint macros with regular functions by MarcoFalke)
#14045 (refactor: Fix the chainparamsbase -> util circular dependency by Empact)
#13098 (Skip tx-rehashing on historic blocks by MarcoFalke)
#11535 (Avoid unintentional unsigned integer wraparounds by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

arvidn commented at 5:12 am on September 6, 2018: none

Is there any document that can prove this? I don’t think they act different when they are in .cpp and .h files

[decl.inline] say:

An inline function or variable shall be defined in every translation unit in which it is odr-used and shall have exactly the same definition in every case ([basic.def.odr]).

which is really the main definition of inline in C++. Since a static function has internal linkage, it will already be defined in every translation unit it appears in.

regarding the distinction of .h vs. .cpp, they do behave the same, however, when a static function appear in a cpp file, there will only ever be one version of it. if a static function appear in a header, it’s likely there will be multiple copies. It’s potentially more space efficient to just have a single copy, that’s why I removed static from headers but not from .cpp files.

practicalswift commented at 2:47 pm on September 6, 2018: contributor

utACK ee258a03b4e14751f5ecb9098a11d90cc32f230e

Excellent first-time contribution! I’m so glad to see you as a Bitcoin Core contributor @arvidn. I love your work on libtorrent and I’m also a fan of your C++ talks.

The high quality standards you strive for in the libtorrent project and enforce by use of automated testing/linting is really impressive. Thanks for some really inspiring work and hope you’ll bring some of that drive for high quality and correctness over here in the form of future contributions!

Welcome to Bitcoin Core! :-)

promag commented at 3:09 pm on September 6, 2018: member

@arvidn I’m curious if you found these with grep or other tool? I mean, is this complete? And can we add check to prevent more “wrong” code?

arvidn commented at 3:24 pm on September 6, 2018: none

@promag good question. I used vim for the bulk of the change, specifically:

0:argadd src/*.cpp src/*/*.cpp
1:argdo %s/static inline /static /ge

and

0:argadd src/*.h src/*/*.h
1:argdo %s/static inline /inline /ge

Then I grepped out the templates from the diff that created.

So, any code outside of those globs was not included.

sipa commented at 4:54 pm on September 6, 2018: member

Furthermore, a static function in a cpp file already has internal linkage, making it inline has no effect.

Templates already have an inline-like property of being allowed to be defined multiple times, and having the linker deal with it. Making them inline is also redundant.

As far as I know, inline has two effects. One is allowing the function to be defined in multiple translation units (as long as they’re all identical). The second is a hint to the compiler (but just a hint) that it may be worthwhile to inline the function at call sites. I’d like to be sure that removing inline does not hurt performance through that second effect.

arvidn commented at 5:35 pm on September 6, 2018: none

@sipa that’s exactly right, those are the two effects. I believe it’s widely understood that all major compilers these days ignore the hint and base inlining entirely on their own heuristics. It’s a fair concern though. I’ll see what I can come up with. Are there any performance regression tests by any chance?

MarcoFalke commented at 5:52 pm on September 6, 2018: member

There is ./src/bench/bench_bitcoin, but it has not full coverage.

arvidn commented at 7:43 pm on September 6, 2018: none

it appears at least clang does take the inline keyword into account in its heuristic [ref]. I wouldn’t expect it to really make a significant difference in this case (either direction).

Perhaps a better approach would be to split this PR into two. I could revert the removal of inline and submit that as a separate PR where I can put some more work into investigating potential performance impact

arvidn commented at 4:42 pm on September 7, 2018: none

I put some work into investigating whether any of the inline decisions changed by this patch. Given that it’s primarily decided by heuristics, this may differ on GCC. I tested this with clang using the -Rpass=inline command line switch. This logs every time the inliner fires.

Specifically I ran:

0CFLAGS=-Rpass=inline ./configure
1make V=1 -j1 2> master-inlining.log
2grep "remark:" -A 3 master-inlining.log >master.log

and then the same thing in this branch (after cleaning obviously):

0make V=1 -j1 2> branch-inlining.log
1grep "remark:" -A 3 branch-inlining.log >branch.log

My hope was that the output would be identical, but it wasn’t for various reasons. However, all the actual inline decisions were the same. The difference was that some names were mangled differently (presumably because of the linkage change) and some logs quoting lines in the source changed, because the source changed.

for example:

0-./primitives/transaction.h:404:116: remark: _ZNSt3__17forwardI19CMutableTransactionEEOT_RNS_16remove_referenceIS2_E4typeE inlined into _Z18MakeTransactionRefI19CMutableTransactionENSt3__110shared_ptrIK12CTransactionEEOT_ [-Rpass=inline]
1+./primitives/transaction.h:404:130: remark: _ZNSt3__17forwardI19CMutableTransactionEEOT_RNS_16remove_referenceIS2_E4typeE inlined into _ZL18MakeTransactionRefI19CMutableTransactionENSt3__110shared_ptrIK12CTransactionEEOT_ [-Rpass=inline]

(note the additional “L” in the mangled name)

0-template <typename Tx> CTransactionRef MakeTransactionRef(Tx&& txIn) { return std::make_shared<const CTransaction>(std::forward<Tx>(txIn)); }
1-                                                                                                                   ^
2+template <typename Tx> static inline CTransactionRef MakeTransactionRef(Tx&& txIn) { return std::make_shared<const CTransaction>(std::forward<Tx>(txIn)); }
3+                                                                                                                                 ^

So, at least for for clang, this patch does not change any inlining decisions.

sipa commented at 5:01 pm on September 8, 2018: member

@arvidn I just observed inline on a member function inside an anonymous namespace having an effect with GCC (version 7.3.0, -O2), in a different project with pretty complicated templated code.

I very much doubt the same effect would be observed in Bitcoin Core (nothing similarly complex happens here), but I thought I’d point it out as it may mean we need to consider GCC as well.

DrahtBot commented at 9:28 am on September 13, 2018: member

DrahtBot added the label Needs rebase on Sep 13, 2018

MarcoFalke commented at 8:12 pm on September 20, 2018: member

Still needs rebase, so closing for now. Let me know when you work on this again, so I can reopen.

MarcoFalke closed this on Sep 20, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021

fix various uses of inline #14158