future: hint need_preempt() accordingly to SEASTAR_DEBUG

[rzarzynski]

Although seastar::need_preempt() is really small in terms of code footprint, it's also frequently used with no code differences between occurrences. Not surprisingly compilers might consider it as a candidate to actually not inline. Moreover, GCC 8.1.0 on Ubuntu has failed to propagate the branch prediction hinting present in the definition:

inline bool need_preempt() noexcept {
    // ...
    return __builtin_expect(head != tail, false);
}

over function's callers.

The topic whether wrapping __builtin_expect() inside inlineable function stops the propagation was discussed on Stack Overflow (see https://stackoverflow.com/a/34427803) but without conclusion for GCC (though Clang forgets it for sure). However, objdump shows the hint has been lost:

  # fragment of crimson-osd's hot path: call the future-returning
  # `PGBackend::read()` and schedule a continuation with `then()`.
  # The future is always ready by the way.
  853b47:       4c 8d bd c0 fe ff ff    lea    -0x140(%rbp),%r15
  ...
  853b57:       4c 89 ff                mov    %r15,%rdi
  ...
  853b6c:       e8 af fb fc ff          callq  823720 <...>
  853b71:       48 83 bd c8 fe ff ff    cmpq   $0x1,-0x138(%rbp)
  853b78:       01
  ...
  853b7b:       0f 86 7f 03 00 00       jbe    853f00 <...>
  853b81:       e8 3a 89 f1 ff          callq  76c4c0 <_ZN7seastar12need_preemptEv>
  853b86:       84 c0                   test   %al,%al
  853b88:       0f 84 32 04 00 00       je     853fc0 <...>

After the patch the same fragment looks in following way:

  854397:       4c 8d bd c0 fe ff ff    lea    -0x140(%rbp),%r15
  ...
  8543a7:       4c 89 ff                mov    %r15,%rdi
  ...
  8543bc:       e8 6f f5 fc ff          callq  823930 <...>
  8543c1:       48 83 bd c8 fe ff ff    cmpq   $0x1,-0x138(%rbp)
  8543c8:       01
  ...
  8543cb:       0f 86 cf 02 00 00       jbe    8546a0 <...>
  8543d1:       e8 ca 89 f1 ff          callq  76cda0 <_ZN7seastar12need_preemptEv>
  8543d6:       84 c0                   test   %al,%al
  8543d8:       0f 85 c9 03 00 00       jne    8547a7 <...>

and the handling of ready, exception-less future is put on the optimistic, fallthrough path. This might have some impact on CPU's frontend but for sure makes reading assembler a little bit easier as humans are much slower with resteers than any CPU. That's also the reason why the hint value depends on SEASTAR_DEBUG.

Signed-off-by: Radoslaw Zarzynski [email protected]

[tchaikov]

also i'd suggest fold these two commits into a single one.

[tchaikov]

also tested with GCC 9.2

before the change:

(gdb) disass need_preempt
Dump of assembler code for function seastar::need_preempt():
=> 0x0000555555630f90 <+0>:     mov    $0xffffffffffffed80,%rax
   0x0000555555630f97 <+7>:     mov    %fs:(%rax),%rax
   0x0000555555630f9b <+11>:    mov    (%rax),%edx
   0x0000555555630f9d <+13>:    mov    0x4(%rax),%eax
   0x0000555555630fa0 <+16>:    cmp    %edx,%eax
   0x0000555555630fa2 <+18>:    setne  %al
   0x0000555555630fa5 <+21>:    retq
End of assembler dump.

so there is no jump at all! 🤣

on the caller site:

src/seastar/include/seastar/core/future.hh

1064                if (available() && !need_preempt()) {

   0x000055555571e452 <+290>:   jbe    0x55555571ee00 <seastar::reactor::configure(boost::program_options::variables_map)+2768>
   0x000055555571e458 <+296>:   callq  0x555555630f90 <seastar::need_preempt()>
=> 0x000055555571e45d <+301>:   test   %al,%al
   0x000055555571e45f <+303>:   jne    0x55555571e4e0 <seastar::reactor::configure(boost::program_options::variables_map)+432>
   0x000055555571e461 <+305>:   mov    -0x238(%rbp),%rax
   0x000055555571e468 <+312>:   mov    -0x240(%rbp),%rdx
   0x000055555571e46f <+319>:   cmp    $0x2,%rax /// inlined available(), compare _u.st with state::result (which is "2")

so the happy/predicted path assumes head == tail. this behavior is expected. but after applying

diff --git a/include/seastar/core/preempt.hh b/include/seastar/core/preempt.hh
index 722c7831..665a4050 100644
--- a/include/seastar/core/preempt.hh
+++ b/include/seastar/core/preempt.hh
@@ -49,7 +49,7 @@ inline bool need_preempt() noexcept {
     auto tail = np->tail.load(std::memory_order_relaxed);
     // Possible optimization: read head and tail in a single 64-bit load,
     // and find a funky way to compare the two 32-bit halves.
-    return __builtin_expect(head != tail, false);
+    return __builtin_expect(head != tail, true);
 #else
     return true;
 #endif

the compiler emits exactly the same assembly.

[rzarzynski]

@tchaikov: need_preempt() looks exactly the same on my machine too (x86-64, Ubuntu, GCC 8.1.0).

After consideration I think we should keep the __builtin_expect() there intact. The reason I see is combination of two factors:

• depending on compiler, targeted microarchitecture (-march=) and so on conditional jump might be emitted instead of setne. There is long but interesting post on StackOverflow about cmov and differences between GCCs.
• The macro targets situations when a compiler decides to actually not inline the function. However, such decisions are made on per-occurance basis – there could be places where inlining happens. I haven't verified propagation of the hint in "the reversed direction".