fix various uses of inline

[arvidn]

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:

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

[ken2812221]

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

[DrahtBot]

Note to reviewers: This pull request conflicts with the following ones:

• logging: Replace LogPrint macros with regular functions #14209 (logging: Replace LogPrint macros with regular functions by MarcoFalke)
• refactor: Fix the chainparamsbase -> util circular dependency #14045 (refactor: Fix the chainparamsbase -> util circular dependency by Empact)
• Skip tx-rehashing on historic blocks #13098 (Skip tx-rehashing on historic blocks by MarcoFalke)
• Avoid unintentional unsigned integer wraparounds #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]

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]

utACK ee258a0

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]

@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]

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

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

and

:argadd src/*.h src/*/*.h
: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]

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]

@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?

[maflcko]

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

[arvidn]

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]

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:

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

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

make V=1 -j1 2> branch-inlining.log
grep "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:

-./primitives/transaction.h:404:116: remark: _ZNSt3__17forwardI19CMutableTransactionEEOT_RNS_16remove_referenceIS2_E4typeE inlined into _Z18MakeTransactionRefI19CMutableTransactionENSt3__110shared_ptrIK12CTransactionEEOT_ [-Rpass=inline]
+./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)

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

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

[sipa]

@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]

Needs rebase

[maflcko]

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