1677553 - Add guidelines on when/where to provide inline function definitions in a header file

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, task)

Description

[Simon Giesecke [:sg] [he/him]]

Since providing inline definitions in a header file often requires additional headers to be included, so providing inline definitions often has a significant build time cost. There should be guidelines on when to do this, and how (e.g. in the main vs. a separate header file, where to put inline, where to include the separate header file if any, for member functions inside or outside the class definition).

Comment 1

[Simon Giesecke [:sg] [he/him]]

The answer might be simple for non-templates in times of LTO as long as one stays in link unit boundaries, e.g. within libxul, and might be: never.

But I am not sure about that. Are there situations where LTO can't inline a function call because there's no "inline" definition available, even when caller and callee are in the same link unit?

Comment 2

[Simon Giesecke [:sg] [he/him]]

There definitely is an effect on inlining decisions in a LTO (non-PGO) build. I moved the definition of Element::ReleasePointerCapture (https://searchfox.org/mozilla-central/rev/ff82c973f8ccb0475ec32439e9ec07014b3a681f/dom/base/Element.h#1227) to the cpp file, and this causes it to be emitted, while it was apparently always inlined before. (FWIW, it's called only at two places in the generated bindings: https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom7Element21ReleasePointerCaptureEiRNS_11ErrorResultE&redirect=false)

But I am unsure what this means. I can't see a reason why LTO should not be able to still inline the calls in principle. But apparently it decides that it should no longer be inlined. What's better in terms of code size vs. performance trade-offs is hard to say in general. So while the decisions might be different, they might be better.

I am still somewhat surprised, because I originally thought it wouldn't make any difference for the inlining decisions. I haven't found any definitive answer to how this is affected. Similar questions is raised several times on SO, e.g. in https://stackoverflow.com/questions/7046547/link-time-optimization-and-inline, but the discussion is about gcc here, not about clang/LLVM.

Comment 3

[Mike Hommey [:glandium]]

You can't rely on LTO providing the same result as inlining from headers, the same that you actually can't rely on inlining from headers depending on the size of your code. You'll probably find that if you have a massive cpp file and a lot of uses of the inline function, the compiler might decide to stop inlining it. The same applies to LTO. The only way I know of to force the compiler to inline is attribute((always_inline)), but I don't know what that does with LTO.

Comment 4

[Simon Giesecke [:sg] [he/him]]

Sure, just providing an inline(able) definition of a function doesn't guarantee it's getting inlined in a non-LTO build. I wasn't thinking that it will get inlined always, I was just thinking that the result is the same with both an non-LTO inline(able) definition and a "hidden" definition. But apparently that's not the case. (Sorry, I hope it's clear what I mean, I am missing an exact terminology here).

The question remaining for me is if we can assume that the toolchain making sensible inlining decisions with a "hidden" definition, at least unless performance measurements show otherwise in a specific case and state a guideline like:

• Never place function definitions in a header file, unless this is syntactically required.

If we could provide this guideline, and it is followed, this would greatly reduce the chances of include-dependency ripple effects...

WDYT?

The only way I know of to force the compiler to inline is attribute((always_inline)), but I don't know what that does with LTO.

I think with at least with PGO, attribute((always_inline)) gets ignored. It's a great question if an "outlined" function attributed with attribute((always_inline)) always gets inlined in a LTO, non-PGO build.

Comment 5

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1677553 - Add guidelines on inline function bodies in header files. r=glandium

Depends on D97916

Comment 6

[(Away)]

Just want to chime in and say that I've never had a problem with MOZ_ALWAYS_INLINE or MOZ_NEVER_INLINE not being respected (PGO or not), and in my experience LTO is not afraid of inlining many levels deep regardless of whether it was explicitly requested or not. There is the possibility of corner cases but I don't want that to be a reason for our guidelines to encourage people to generally distrust the tools. I like the phrasing in the proposed patch about not doing things for perf unless there's evidence, that's a good way to put it.

Comment 7

[Andi [:andi]]

In the end we shouldn't do this because it;s a little bit too much of wishful thinking regarding LTO.

Comment 8

[Marco Castelluccio [:marco]]

(In reply to Andi [:andi] from comment #7)

In the end we shouldn't do this [...]

WONTFIX is a better resolution then :)