Inlining functions

[buschbapti]

There was a discussion regarding inlining functions in PR #52. I checked a bit more and, indeed, functions that are defined in a separate source file are never inlined. So the question becomes then do we think inlining might be useful or not. I can see for getter and setters but it seems performances are not that much improved.

If we want to inline functions but do not want to cluter the actual header file, one practice seems to be the usage of a .inl file https://www.oreilly.com/library/view/c-cookbook/0596007612/ch02s06.html. This could also be used for template implementations. What are your opinions on that?

[eeberhard]

Without good benchmarking it's hard to say in any case what costs and benefits manual inlining has on the execution size and time of our library.

Here are my thoughts in a somewhat unorganised fashion.

Thoughts about inlining

Inlining a function within a single translation unit is nice because you know in advance how many times it could be repeated. But for public inline functions, we can only guess what the end-user will do. For concerns about speed, it's worth thinking about what operations might be called once or twice in a high-frequency control loop, and which operations will be called many times across the code base. The former usage (high-frequency in a few locations) would probably benefit from inlining. The latter case (low-frequency in time but high-frequency in the logic) would probably not benefit from inlining.

But, because the compiler is always free to ignore the inlining directive, I suppose neither case can ever be worse as a result of inlining. Conversely, the compiler is also free to inline a function that is not inlined, provided its definition is available outside of the translation unit. So, the question really is about where potential gains would warrant putting a function definition in the header file, and that would be in the first case (high-frequency usage).

This leads into getters and setters. I agree that simple calls would likely be optimised inline by the compiler if they are available. For pointers and parameters, I agree that A.set_b(c) is nicer and more consistent than A.b->set_value(c), and so can be a setter. Inlining this also makes some sense to me.
For more simple s/getters that just access the class property directly, I think that should just be a public property instead of a function, which removes the need for a definition anyway.
For more complex s/getters, and in general for main class functions, I think the complier would probably not inline them anyway, particularly if they internally call other subfunctions whose implementations cannot be expanded.

Assuming that we can maybe get some marginal performance gains from inlining some implementations in the header, this leads to the next question about "header bloat".

More files?

I tried to think of some reasons why it's good reduce the size of header files:

1. To make headers more readable and act as a kind of API documentation (mainly for developers).
2. To simplify / reduce the size of the library include dependencies.
3. To hide implementation code from end-users.

There is something nice about proposing a new file type for this case. .hpp files contain only class and function declarations. .inl files contain definitions for public and protected inline functions as well as definitions for templated functions. .cpp files contain all other definitions. This would somewhat address the first point, but not actually 2 or 3. This is because the installation of the library will have to copy and resolve these .inl files. This would actually make the "size" of the public API bigger, not smaller. It also might be even more daunting to new contributors to see this unfamiliar pattern.

There are some additional counterpoints to the above reasons:

1. Doxygen gives deep API information, so why should anyone be looking at the source code for API patterns?
2. This is open-source so there's not really anything to hide.

So if .inl solution might only benefit point 1 (but we have a better documentation framework in place already), slightly worsens point 2, and doesn't really affect point 3, and potentially deters new collaborators, is it worth doing at all? I don't know

Some kind of conclusion

I think the main step is to re-evaluate and reduce public methods, starting with simplifying getters/setters. This will already reduce header size and the amount of inlining keywords. Beyond that we might want some performance benchmarks (not to be run on every PR but as an occasional option, perhaps on each release) so we can actually see where efforts to optimise and "outsmart" the compiler are actually warranted. But probably the consequences to moving some of the function implementations into the class source files are not going to be so significant.

[buschbapti]

Thanks for the indepth analysis. Indeed without benchmarking it is rather difficult to evaluate. Anyway, the refactor to avoid mutable states will remove its fair load of setters / getters and we can remove most of the inline at that time. I somehow like the .inl file but if not really common it might indeed confuse developers. I think the easiest would actually be to remove all the inlines

[domire8]

Interesting thoughts as always, I'm following this discussion closely but I can't add something valuable because I have no experience with that 😄

[eeberhard]

I found another potential issue with inlining functions - see #75

The problem may be unrelated to the inline, I will need to do some more tests, but it seems plausible to me