From 9939aef6248520e7642e27ac82d65313144aed15 Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Thu, 24 Oct 2019 11:26:57 +0200 Subject: [PATCH 1/2] future: hint need_preempt() accordingly to SEASTAR_DEBUG. 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: ```cpp 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 (but 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 hot, 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 --- include/seastar/core/future-util.hh | 14 +++++++------- include/seastar/core/future.hh | 6 +++--- include/seastar/core/preempt.hh | 6 ++++++ 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/include/seastar/core/future-util.hh b/include/seastar/core/future-util.hh index 874932ffd73..a362bffdf0c 100644 --- a/include/seastar/core/future-util.hh +++ b/include/seastar/core/future-util.hh @@ -224,7 +224,7 @@ public: _promise.set_value(); return; } - } while (!need_preempt()); + } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); } catch (...) { _promise.set_exception(std::current_exception()); return; @@ -297,7 +297,7 @@ future<> repeat(AsyncAction action) { if (f.get0() == stop_iteration::yes) { return make_ready_future<>(); } - } while (!need_preempt()); + } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); auto repeater = std::make_unique>(stop_iteration::no, std::move(futurized_action)); auto ret = repeater->get_future(); @@ -370,7 +370,7 @@ public: _promise.set_value(std::make_tuple(std::move(*ret))); return; } - } while (!need_preempt()); + } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); } catch (...) { _promise.set_exception(std::current_exception()); return; @@ -427,7 +427,7 @@ repeat_until_value(AsyncAction action) { if (optional) { return make_ready_future(std::move(optional.value())); } - } while (!need_preempt()); + } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); try { auto state = std::make_unique>(compat::nullopt, std::move(futurized_action)); @@ -473,7 +473,7 @@ public: f.forward_to(std::move(_promise)); return; } - } while (!need_preempt()); + } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); } catch (...) { _promise.set_exception(std::current_exception()); return; @@ -517,7 +517,7 @@ future<> do_until(StopCondition stop_cond, AsyncAction action) { if (f.failed()) { return f; } - } while (!need_preempt()); + } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); auto task = std::make_unique>(std::move(stop_cond), std::move(action)); auto f = task->get_future(); @@ -570,7 +570,7 @@ future<> do_for_each(Iterator begin, Iterator end, AsyncAction action) { if (begin == end) { return f; } - if (!f.available() || need_preempt()) { + if (!f.available() || __builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)) { return std::move(f).then([action = std::move(action), begin = std::move(begin), end = std::move(end)] () mutable { return do_for_each(std::move(begin), std::move(end), std::move(action)); diff --git a/include/seastar/core/future.hh b/include/seastar/core/future.hh index a8882e37056..2cb816ed028 100644 --- a/include/seastar/core/future.hh +++ b/include/seastar/core/future.hh @@ -962,7 +962,7 @@ private: Result then_impl(Func&& func) noexcept { using futurator = futurize>; - if (available() && !need_preempt()) { + if (available() && !__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)) { if (failed()) { return futurator::make_exception_future(get_available_state().get_exception()); } else { @@ -1023,7 +1023,7 @@ private: Result then_wrapped_impl(Func&& func) noexcept { using futurator = futurize>; - if (available() && !need_preempt()) { + if (available() && !__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)) { return futurator::apply(std::forward(func), future(get_available_state())); } typename futurator::promise_type pr; @@ -1262,7 +1262,7 @@ inline void promise::make_ready() noexcept { if (_task) { _state = nullptr; - if (Urgent == urgent::yes && !need_preempt()) { + if (Urgent == urgent::yes && !__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)) { ::seastar::schedule_urgent(std::move(_task)); } else { ::seastar::schedule(std::move(_task)); diff --git a/include/seastar/core/preempt.hh b/include/seastar/core/preempt.hh index e7799661462..7dcd6542c1e 100644 --- a/include/seastar/core/preempt.hh +++ b/include/seastar/core/preempt.hh @@ -22,6 +22,12 @@ #pragma once #include +#ifndef SEASTAR_DEBUG +#define NEED_PREEMPT_EXPECTED false +#else +#define NEED_PREEMPT_EXPECTED true +#endif + namespace seastar { namespace internal { From 817938b41ad5b119a74f34d5cb980ff2024e4e6c Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Thu, 24 Oct 2019 16:32:19 +0200 Subject: [PATCH 2/2] future, core: convert need_preempt() into a macro. Signed-off-by: Radoslaw Zarzynski --- include/seastar/core/future-util.hh | 14 +++++++------- include/seastar/core/future.hh | 6 +++--- include/seastar/core/preempt.hh | 26 ++++++++++++++------------ 3 files changed, 24 insertions(+), 22 deletions(-) diff --git a/include/seastar/core/future-util.hh b/include/seastar/core/future-util.hh index a362bffdf0c..874932ffd73 100644 --- a/include/seastar/core/future-util.hh +++ b/include/seastar/core/future-util.hh @@ -224,7 +224,7 @@ public: _promise.set_value(); return; } - } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); + } while (!need_preempt()); } catch (...) { _promise.set_exception(std::current_exception()); return; @@ -297,7 +297,7 @@ future<> repeat(AsyncAction action) { if (f.get0() == stop_iteration::yes) { return make_ready_future<>(); } - } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); + } while (!need_preempt()); auto repeater = std::make_unique>(stop_iteration::no, std::move(futurized_action)); auto ret = repeater->get_future(); @@ -370,7 +370,7 @@ public: _promise.set_value(std::make_tuple(std::move(*ret))); return; } - } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); + } while (!need_preempt()); } catch (...) { _promise.set_exception(std::current_exception()); return; @@ -427,7 +427,7 @@ repeat_until_value(AsyncAction action) { if (optional) { return make_ready_future(std::move(optional.value())); } - } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); + } while (!need_preempt()); try { auto state = std::make_unique>(compat::nullopt, std::move(futurized_action)); @@ -473,7 +473,7 @@ public: f.forward_to(std::move(_promise)); return; } - } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); + } while (!need_preempt()); } catch (...) { _promise.set_exception(std::current_exception()); return; @@ -517,7 +517,7 @@ future<> do_until(StopCondition stop_cond, AsyncAction action) { if (f.failed()) { return f; } - } while (!__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)); + } while (!need_preempt()); auto task = std::make_unique>(std::move(stop_cond), std::move(action)); auto f = task->get_future(); @@ -570,7 +570,7 @@ future<> do_for_each(Iterator begin, Iterator end, AsyncAction action) { if (begin == end) { return f; } - if (!f.available() || __builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)) { + if (!f.available() || need_preempt()) { return std::move(f).then([action = std::move(action), begin = std::move(begin), end = std::move(end)] () mutable { return do_for_each(std::move(begin), std::move(end), std::move(action)); diff --git a/include/seastar/core/future.hh b/include/seastar/core/future.hh index 2cb816ed028..a8882e37056 100644 --- a/include/seastar/core/future.hh +++ b/include/seastar/core/future.hh @@ -962,7 +962,7 @@ private: Result then_impl(Func&& func) noexcept { using futurator = futurize>; - if (available() && !__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)) { + if (available() && !need_preempt()) { if (failed()) { return futurator::make_exception_future(get_available_state().get_exception()); } else { @@ -1023,7 +1023,7 @@ private: Result then_wrapped_impl(Func&& func) noexcept { using futurator = futurize>; - if (available() && !__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)) { + if (available() && !need_preempt()) { return futurator::apply(std::forward(func), future(get_available_state())); } typename futurator::promise_type pr; @@ -1262,7 +1262,7 @@ inline void promise::make_ready() noexcept { if (_task) { _state = nullptr; - if (Urgent == urgent::yes && !__builtin_expect(need_preempt(), NEED_PREEMPT_EXPECTED)) { + if (Urgent == urgent::yes && !need_preempt()) { ::seastar::schedule_urgent(std::move(_task)); } else { ::seastar::schedule(std::move(_task)); diff --git a/include/seastar/core/preempt.hh b/include/seastar/core/preempt.hh index 7dcd6542c1e..aed21a8b728 100644 --- a/include/seastar/core/preempt.hh +++ b/include/seastar/core/preempt.hh @@ -22,12 +22,6 @@ #pragma once #include -#ifndef SEASTAR_DEBUG -#define NEED_PREEMPT_EXPECTED false -#else -#define NEED_PREEMPT_EXPECTED true -#endif - namespace seastar { namespace internal { @@ -40,12 +34,14 @@ struct preemption_monitor { std::atomic tail; }; -} +} // namespace internal extern __thread const internal::preemption_monitor* g_need_preempt; -inline bool need_preempt() { +namespace internal { + #ifndef SEASTAR_DEBUG +inline bool _need_preempt() { // prevent compiler from eliminating loads in a loop std::atomic_signal_fence(std::memory_order_seq_cst); auto np = g_need_preempt; @@ -56,9 +52,15 @@ inline bool need_preempt() { // 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); -#else - return true; -#endif } +#endif // not SEASTAR_DEBUG -} +} // namespace internal + +} // namespace seastar + +#ifndef SEASTAR_DEBUG +#define need_preempt() __builtin_expect(::seastar::internal::_need_preempt(), false) +#else +#define need_preempt() true +#endif // not SEASTAR_DEBUG