Boost C++ Libraries: Ticket #5735: proto should force functions to be inline

status changed; resolution set

Eric Niebler — Tue, 26 Jul 2011 19:59:38 GMT

status new → closed
resolution → invalid

This is none of Proto's business. If there is a compiler bug leading it to poorly inline, then it sould be fixed in the compiler, not worked around in Proto. Forcing inlining might actually degrade performance for compilers that have good inlining heuristics.

Mathias Gaunard — Tue, 26 Jul 2011 21:24:06 GMT

Since Proto is used to build many performance sensitive libraries and applications, and there is no downside to implementing this feature, I don't understand why you're discarding this request so easily.

Proto is a library for others to build upon, each having their own uses, requirements, compilers and toolchains. No compiler is perfect, and inlining is certainly not a perfect feature and is very dependent of the flags used -- which themselves may have other negative effects. That means that you cannot rely on the compiler to do it reliably, especially when large codebases are involved. There are always inlining issues, different ones with each version of GCC for example, including recent and widely deployed ones.

Some Proto constructs make no sense to not be inlined, so it seems like a good idea to tell the compiler that yes, they should always be inlined, and if it cannot for some reason, it should emit an error. It is also important in order to get decent performance when running the code in debug mode or in modes that are not too aggressive with inlining, such as size-optimized builds, popular on embedded systems.

I believe doing this would only improve the quality of Proto and make people more eager to rely on it.

I think the following paper, in section 9, sums up the importance of inlining with expression templates pretty well: http://arxiv.org/pdf/1104.1729

Eric Niebler — Tue, 26 Jul 2011 21:57:34 GMT

Wow, the paper is from this year, and they think that Blitz++ and uBLAS represent the bleeding edge of ET technology. That's laughable.

But the perf measurements are interesting. I can't see where they say what compiler they're using for their perf benchmarks, can you? If they're using that buggy GCC compler version, then yes, it will grossly screw up the results. They talk about cache issues, but only consider data size and not code size.

Joel Falcou is the one closest to this issue. He's filed specific bugs against Proto (function X should be changed in way Y for better inlining), which I have addressed promptly. He hasn't noticed any problems that would require any across-the-board change like the one you're suggesting. Or if he has, he hasn't said anything about it. If you can show me a specific case where a (non-buggy, mainstream) compiler generates slow Proto code because of failed inlining, I'd be more easily convinced.

But you should bring this up on the Proto list and make sure Joel sees it. I'd be curious to hear his thoughts.

type changed

Mathias Gaunard — Tue, 26 Jul 2011 23:05:52 GMT

type Bugs → Feature Requests

Yes, that paper is "not terribly good", yet it has a couple of wise things in it, and the conclusion is interesting, which is why I put it there. (it's from Thomas Heller's advisor, by the way)
I unfortunately don't have more information on how they got those results.

I haven't personally run into problems with inlining and Boost.Proto, since I haven't done things that were very performance critical with it yet that required me to look at the produced asm, but I have run into issues in NT2 (in the non-ET parts) where some very small and trivial functions didn't get inlined on some GCC versions or settings, which was catastrophic to performance (hundreds of times slower).
Some other NT2 users ran into similar problems on MSVC, albeit on different functions.

I believe putting force inline attributes to functions is an even more reliable solution than making code "compiler-friendly", since this way we can have a guarantee that things will be inlined rather than analyzing asm and finding out a workaround on a case by case basis.

I'm only suggesting that a BOOST_PROTO_FORCE_INLINE be put instead of inline in the cases where it appears to be a good idea. Is that considered to be too intrusive in the source code?

In any case I'll get Joel in the loop.

anonymous — Wed, 27 Jul 2011 07:55:57 GMT

Replying to eric_niebler:

Wow, the paper is from this year, and they think that Blitz++ and uBLAS represent the bleeding edge of ET technology. That's laughable.

Yeah quite sad ...

As for the problem at hand, I have mgaunard handy in my office, and as he said, we got some sporadic MSVC 10 with some size aggressive optimization. Now, I dont think we need an all-over-your-face approach but just have a force inline macro handy that me/you can apply where and when necessary.

Mathias Gaunard — Tue, 01 Nov 2011 13:39:50 GMT

I suggest at least proto::value(), proto::child, make_expr and related transforms be marked force inline. Looking at assembly, it happens relatively often that boost::proto::value is not inlined, even though it definitely should always be.

boost/config.hpp now contains a BOOST_FORCEINLINE macro that can be used for this purpose.

attachment set

Mathias Gaunard — Wed, 02 Nov 2011 11:06:04 GMT

attachment → proto_forceinline.tar.gz

Adds BOOST_FORCEINLINE in relevant parts of Boost.Proto

Mathias Gaunard — Wed, 02 Nov 2011 11:15:19 GMT

The attached patch adds BOOST_FORCEINLINE to the following functions:

as_lvalue, ignore_unused, dont_care
as_child, as_expr
proto_base, address_of_hack
child, child_c, value
make_expr, unpack_expr
operators and operators defined by extends, extends constructor
built-in generators
the transform operator() overloads that give dummy state and data
call, make, when and pass_through transforms

This patch adds a significant number of BOOST_FORCEINLINE, but all of them make sense, because the functions in question are lightweight wrappers or only forward non-recursively to another function.

status, version changed; resolution deleted

Eric Niebler — Sun, 20 Nov 2011 21:48:39 GMT

status closed → reopened
version Boost 1.47.0 → Boost 1.49.0
resolution invalid

This is fixed on trunk in [75578]. I'll merge this to release after the tests have cycles. Thanks, Mathias.

status changed; resolution set

Eric Niebler — Sat, 17 Dec 2011 19:17:46 GMT

status reopened → closed
resolution → fixed

(In [76026]) merge [75578] from trunk, fixes #5735