[Development] HEADS-UP: Q_UNLIKELY

[Thiago Macieira]

On Friday, 30 August 2019 06:31:29 PDT Mutz, Marc via Development wrote:
> 2. As a separate ELF section, barring relocations etc that might prevent
> this, these .cold sections will stay parked on disk and loaded into
> memory only on demand, which means that executing unlikely code may make
> the program actually very slow (like exceptions, which on modern ABIs
> have zero runtime overhead when not thrown but may load debug symbols to
> unwind the stack when thrown).

There are no relocations in code and there haven't been for decades. Their 
existence causes warnings in the linker and Linux distributions flag them. The 
ELF header gets a DT_TEXTREL because of that.

Position-Independent Code is achieved by having the relocations in a separate 
table, which is called the Global Offset Table (GOT). That way, all the 
relocations are dense: it's pagefuls of relocated addresses, as opposed to 
accidentally causing a page to be dirty/unsharable because of a single 
relocation.

So, no, relocations will not cause the .text.unlikely section to load.

> This is not something we can control (that I know of, other than telling
> Ville to change it), so we don't need to discuss whether that's good or
> bad. It simply _is_.

There's also the question of whether the OS would have proactively loaded that 
page. First, the OS doesn't know where the active code will be until it starts 
executing, so it may decide to load everything. Second, the .text.unlikely 
section is going to be bracketed by loaded pages (regular .text before and 
.rodata after), so the OS may decide to load that or keep it in memory.

> In turn, this means that we should no longer use Q_UNLIKELY (I don't
> know about Q_LIKELY, but am willing to be enlightened) to mark code that

#  define Q_LIKELY(expr)    __builtin_expect(!!(expr), true)
#  define Q_UNLIKELY(expr)  __builtin_expect(!!(expr), false)

There's little difference between the two. Every time you use either, then one 
code path is likely and the other is unlikely.

> gets executed 0.01% of the time, but with very high chance _is_ executed
> in normal program flow once, because that causes all the error handling
> code to be paged in. We should use it only (but then we should strive to
> actually do so) for actual error code. A good indicator is if a
> qWarning/qFatal/qCritical is executed: that's error code.

Interestingly, that's one case you don't need to.

Take a look at <https://gcc.godbolt.org/z/H-oWrl>. As you can see, GCC split 
the f() function in two, with an unlikely section ("clone .cold"), simply 
because it is calling a cold function. Our warning, critical and fatal 
functions are all marked cold. That was done in commit 
c6497e3eac1ac81497f02b40ea7f140a997b4f29 by a certain Marc Mutz :-)

Unfortunately, other compilers don't do that: https://gcc.godbolt.org/z/pXFibR
But it's not a loss because they don't have .text.unlikely sections anyway.

> doesn't work with older GCCs which claim [[unlikely]] support, so we
> will need to either add compiler version checks, or yet another macro,
> or restrict the patterns to the known-good ones:
> 
>     Q_UNLIKELY_CASE case-label:

We'd need a macro anyway because of older compilers that don't support the 
attribute. The older C++11 rule was that they should complain on attributes 
they don't understand anyway.

I don't mind adding the macro now.

But I do mind littering our code with it. It's of very limited value so far 
since few compilers support it. And using the Q_LIKELY/Q_UNLIKELY macros is 
already prone to error: as you've pointed out, a since one of those getting 
executed causes a page to be loaded that wouldn't otherwise be.

The correct way to deal with that is profile-guided optimisation. Compile Qt 
with instrumentation, run some complex but representative applications, then 
feed the profile results back into GCC. That way, it will organise the code to 
optimise I-Cache usage and move actually unlikely code as well as only-used-
once code to separate pages.
-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel System Software Products