From 234c9ac44c389974391cd9701da3ab8531d0a002 Mon Sep 17 00:00:00 2001 From: chenBright Date: Wed, 16 Sep 2026 22:53:01 +0800 Subject: [PATCH 1/4] Fix ineffective enable_if constraints in bvar and butil --- src/butil/containers/optional.h | 9 ++-- src/butil/memory/scope_guard.h | 4 +- src/bvar/reducer.h | 88 ++++++++++++++++++++++++--------- test/bvar_reducer_unittest.cpp | 43 ++++++++++++++++ 4 files changed, 115 insertions(+), 29 deletions(-) diff --git a/src/butil/containers/optional.h b/src/butil/containers/optional.h index bde0ad2f3c..99be2ed65b 100644 --- a/src/butil/containers/optional.h +++ b/src/butil/containers/optional.h @@ -199,14 +199,15 @@ class optional { _storage.Init(std::move(value)); } - template ::value>* = nullptr> - explicit optional(const in_place_t, Args&&... args) : _engaged(true) { + template ::value, bool>::type = false> + explicit optional(in_place_t, Args&&... args) : _engaged(true) { _storage.Init(std::forward(args)...); } template &, Args&&...>::value>::type> + std::is_constructible&, Args&&...>::value, + bool>::type = false> optional(in_place_t, std::initializer_list il, Args&&... args) : _engaged(true) { _storage.Init(il, std::forward(args)...); diff --git a/src/butil/memory/scope_guard.h b/src/butil/memory/scope_guard.h index 377819b5db..bd52b1487f 100644 --- a/src/butil/memory/scope_guard.h +++ b/src/butil/memory/scope_guard.h @@ -24,7 +24,7 @@ namespace butil { template::value>> + typename = typename std::enable_if::value>::type> class ScopeGuard; template @@ -33,7 +33,7 @@ ScopeGuard MakeScopeGuard(Callback&& callback) noexcept; // ScopeGuard is a simple implementation to guarantee that // a function is executed upon leaving the current scope. template -class ScopeGuard { +class ScopeGuard { public: ScopeGuard(ScopeGuard&& other) noexcept : _callback(std::move(other._callback)) diff --git a/src/bvar/reducer.h b/src/bvar/reducer.h index 9fdb1960b7..5b5afbd612 100644 --- a/src/bvar/reducer.h +++ b/src/bvar/reducer.h @@ -30,6 +30,7 @@ #include "bvar/detail/series.h" #include "bvar/window.h" #if WITH_BABYLON_COUNTER +#include // std::max std::min #include "babylon/concurrent/counter.h" #endif // WITH_BABYLON_COUNTER @@ -50,6 +51,33 @@ class SeriesSamplerImpl : public Sampler { }; #if WITH_BABYLON_COUNTER +// babylon counters constrain their value type with a static_assert inside the +// class body, which is a hard error rather than a substitution failure. Probing +// them with std::is_constructible<> hence breaks the build for the types they +// reject, instead of falling back to the generic implementation. Mirror the +// constraint here (see babylon/concurrent/counter.h) so that a rejected type is +// never used to instantiate a babylon counter. +// NOTE: the constraint has to be checked in two steps rather than in a single +// expression, because sizeof() can not be applied to void or to an incomplete +// type, and `&&' does not help: short-circuiting is about evaluation, an +// ill-formed sizeof() is still an error. +template ::value || + std::is_floating_point::value> +struct IsBabylonCounterSupported : std::false_type {}; + +template +struct IsBabylonCounterSupported + : butil::integral_constant {}; + +// `void` if the babylon counter supports T, a substitution failure otherwise. +// Selects the babylon-backed partial specializations of Adder/Maxer/Miner below. +// NOTE: resolving to the *type* is a MUST. std::enable_if itself is a type +// no matter what `cond` is, and Adder means Adder, so specializing on +// std::enable_if instead of its ::type silently never matches. +template +using EnableIfBabylonCounter = + typename std::enable_if::value>::type; + template class BabylonVariable: public Variable { public: @@ -93,17 +121,18 @@ class BabylonVariable: public Variable { } T get_value() const { + CHECK(!(butil::is_same::value) || nullptr == _sampler) + << "You should not call Reducer<" << butil::class_name_str() + << ", " << butil::class_name_str() << ">::get_value() when a" + << " Window<> is used because the operator does not have inverse."; return _counter.value(); } T reset() { - if (BAIDU_UNLIKELY((!butil::is_same::value))) { - CHECK(false) << "You should not call Reducer<" << butil::class_name_str() - << ", " << butil::class_name_str() << ">::get_value() when a" - << " Window<> is used because the operator does not have inverse."; - return get_value(); - } - + // Unlike AgentCombiner::reset_all_agents(), reading and clearing the babylon + // counter are two separate steps, so values added in between are lost. This + // only affects explicit reset() by users: sampling of an operator without + // inverse is the only internal user and it runs in a single thread. T result = _counter.value(); _counter.reset(); return result; @@ -370,15 +399,13 @@ class Adder : public Reducer, detail::MinusFrom > { #if WITH_BABYLON_COUNTER // Numerical types supported by babylon counter. template -class Adder>::value>> +class Adder> : public detail::BabylonVariable, detail::AddTo, detail::MinusFrom> { public: typedef T value_type; -private: typedef detail::BabylonVariable, detail::AddTo, detail::MinusFrom> Base; -public: typedef detail::AddTo Op; typedef detail::MinusFrom InvOp; typedef typename Base::sampler_type sampler_type; @@ -449,28 +476,25 @@ class ConcurrentMaxer : public babylon::GenericsConcurrentMaxer { ConcurrentMaxer(T default_value) : _default_value(default_value) {} T value() const { - T result; - if (!Base::value(result)) { - return _default_value; - } + // Base::value() leaves `result' untouched if nothing was counted. + T result = _default_value; + Base::value(result); return std::max(result, _default_value); } private: - T _default_value{0}; + T _default_value{std::numeric_limits::min()}; }; } // namespace detail // Numerical types supported by babylon counter. template -class Maxer>::value>> +class Maxer> : public detail::BabylonVariable, detail::MaxTo, detail::VoidOp> { public: typedef T value_type; -private: typedef detail::BabylonVariable, detail::MaxTo, detail::VoidOp> Base; -public: typedef detail::MaxTo Op; typedef detail::VoidOp InvOp; typedef typename Base::sampler_type sampler_type; @@ -527,17 +551,35 @@ class Miner : public Reducer > { }; #if WITH_BABYLON_COUNTER +namespace detail { +// The min counterpart of ConcurrentMaxer, see there for why the default value is +// needed. +template +class ConcurrentMiner : public babylon::GenericsConcurrentMiner { + typedef babylon::GenericsConcurrentMiner Base; +public: + ConcurrentMiner() = default; + explicit ConcurrentMiner(T default_value) : _default_value(default_value) {} + + T value() const { + T result = _default_value; + Base::value(result); + return std::min(result, _default_value); + } +private: + T _default_value{std::numeric_limits::max()}; +}; +} // namespace detail + // Numerical types supported by babylon counter. template -class Miner>::value>> - : public detail::BabylonVariable, +class Miner> + : public detail::BabylonVariable, detail::MinTo, detail::VoidOp> { public: typedef T value_type; -private: - typedef detail::BabylonVariable, + typedef detail::BabylonVariable, detail::MinTo, detail::VoidOp> Base; -public: typedef detail::MinTo Op; typedef detail::VoidOp InvOp; typedef typename Base::sampler_type sampler_type; diff --git a/test/bvar_reducer_unittest.cpp b/test/bvar_reducer_unittest.cpp index 02923d92c5..cd91a8c0a2 100644 --- a/test/bvar_reducer_unittest.cpp +++ b/test/bvar_reducer_unittest.cpp @@ -279,6 +279,49 @@ TEST_F(ReducerTest, non_primitive) { ASSERT_EQ(9, adder.get_value().x); } +// The generic Adder/Maxer/Miner keep their data in a shared AgentCombiner and +// hence expose share_combiner(), while the babylon-backed ones keep a value-type +// counter and do not. This tells the two implementations apart. +template +struct IsBabylonBacked + : std::integral_constant::value> {}; + +// Which implementation backs a given value type is decided by SFINAE at compile +// time, so a malformed condition silently makes the babylon-backed partial +// specializations unreachable without failing any runtime assertion. Assert the +// selection statically. +TEST_F(ReducerTest, babylon_counter_backend) { +#if WITH_BABYLON_COUNTER + static_assert(IsBabylonBacked >::value, + "Adder should be backed by a babylon counter"); + static_assert(IsBabylonBacked >::value, + "Adder should be backed by a babylon counter"); + static_assert(IsBabylonBacked >::value, + "Adder should be backed by a babylon counter"); + static_assert(IsBabylonBacked >::value, + "Maxer should be backed by a babylon counter"); + static_assert(IsBabylonBacked >::value, + "Miner should be backed by a babylon counter"); + // babylon counters only support arithmetic types not larger than 8 bytes. + // The types they reject must fall back to the generic implementation rather + // than break the build. + static_assert(IsBabylonBacked >::value == + (sizeof(long double) <= 8), + "Adder should follow the size constraint"); + static_assert(!IsBabylonBacked >::value, + "Adder should be backed by an AgentCombiner"); + static_assert(!IsBabylonBacked >::value, + "Adder should be backed by an AgentCombiner"); +#else + static_assert(!IsBabylonBacked >::value, + "Adder should be backed by an AgentCombiner"); + static_assert(!IsBabylonBacked >::value, + "Maxer should be backed by an AgentCombiner"); + static_assert(!IsBabylonBacked >::value, + "Miner should be backed by an AgentCombiner"); +#endif // WITH_BABYLON_COUNTER +} + bool g_stop = false; struct StringAppenderResult { int count; From 8f3ec497d1c9b74d0d3ca40c18e59d4745b135fd Mon Sep 17 00:00:00 2001 From: chenBright Date: Wed, 16 Sep 2026 23:54:01 +0800 Subject: [PATCH 2/4] Add reset UT --- test/bvar_reducer_unittest.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/bvar_reducer_unittest.cpp b/test/bvar_reducer_unittest.cpp index cd91a8c0a2..f905e110d1 100644 --- a/test/bvar_reducer_unittest.cpp +++ b/test/bvar_reducer_unittest.cpp @@ -322,6 +322,29 @@ TEST_F(ReducerTest, babylon_counter_backend) { #endif // WITH_BABYLON_COUNTER } +// reset() must return the accumulated value and restore the identity of the +// operator, which is how Window<> samples an operator without inverse. This runs +// against whichever implementation backs the value type, hence it covers the +// babylon counters as well when WITH_BABYLON_COUNTER is enabled. +TEST_F(ReducerTest, reset) { + bvar::Adder adder; + adder << 3 << 1; + ASSERT_EQ(4, adder.reset()); + ASSERT_EQ(0, adder.get_value()); + adder << 2; + ASSERT_EQ(2, adder.reset()); + + bvar::Maxer maxer; + maxer << 3 << 1; + ASSERT_EQ(3, maxer.reset()); + ASSERT_EQ(std::numeric_limits::min(), maxer.get_value()); + + bvar::Miner miner; + miner << 3 << 1; + ASSERT_EQ(1, miner.reset()); + ASSERT_EQ(std::numeric_limits::max(), miner.get_value()); +} + bool g_stop = false; struct StringAppenderResult { int count; From b5a83bf0a5d592083a9450f6afdb8dcd0a809b0f Mon Sep 17 00:00:00 2001 From: chenBright Date: Wed, 16 Sep 2026 23:55:01 +0800 Subject: [PATCH 3/4] Use std::enable_if_t instead of std::enable_if::type --- src/butil/containers/optional.h | 46 ++++++++++++++++----------------- src/butil/memory/scope_guard.h | 2 +- src/bvar/reducer.h | 12 ++++----- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/butil/containers/optional.h b/src/butil/containers/optional.h index 99be2ed65b..e196958496 100644 --- a/src/butil/containers/optional.h +++ b/src/butil/containers/optional.h @@ -22,7 +22,7 @@ #include "butil/memory/manual_constructor.h" // The `optional` for managing an optional contained value, -// i.e. a value that may or may not be present, is a C++11 +// i.e. a value that may or may not be present, is a C++14 // compatible version of the C++17 `std::optional` abstraction. // After C++17, `optional` is an alias for `std::optional`. @@ -145,33 +145,33 @@ class optional { optional(optional&& rhs) noexcept = default; - template ::value && std::is_constructible::value && !internal::is_constructible_convertible_from_optional::value && - std::is_convertible::value, bool>::type = false> + std::is_convertible::value, bool> = false> optional(const optional& rhs) : _engaged(rhs.has_value()) { if (_engaged) { _storage.Init(*rhs); } } - template ::value && std::is_constructible::value && !internal::is_constructible_convertible_from_optional::value && - !std::is_convertible::value, bool>::type = false> + !std::is_convertible::value, bool> = false> explicit optional(const optional& rhs) : _engaged(rhs.has_value()) { if (_engaged) { _storage.Init(*rhs); } } - template ::value && std::is_constructible::value && !internal::is_constructible_convertible_from_optional::value && - std::is_convertible::value, bool>::type = false> + std::is_convertible::value, bool> = false> optional(optional&& rhs) : _engaged(rhs.has_value()) { if (_engaged) { _storage.Init(std::move(*rhs)); @@ -179,11 +179,11 @@ class optional { } } - template ::value && std::is_constructible::value && !internal::is_constructible_convertible_from_optional::value && - !std::is_convertible::value, bool>::type = false> + !std::is_convertible::value, bool> = false> explicit optional(optional&& rhs) : _engaged(rhs.has_value()) { if (_engaged) { _storage.Init(std::move(*rhs)); @@ -199,34 +199,34 @@ class optional { _storage.Init(std::move(value)); } - template ::value, bool>::type = false> + template ::value, bool> = false> explicit optional(in_place_t, Args&&... args) : _engaged(true) { _storage.Init(std::forward(args)...); } - template &, Args&&...>::value, - bool>::type = false> + bool> = false> optional(in_place_t, std::initializer_list il, Args&&... args) : _engaged(true) { _storage.Init(il, std::forward(args)...); } - template ::type>::value && !std::is_same, typename std::decay::type>::value && std::is_constructible::value && - std::is_convertible::value, bool>::type = false> + std::is_convertible::value, bool> = false> optional(U&& v) : _engaged(true) { _storage.Init(std::forward(v)); } - template ::type>::value && !std::is_same, typename std::decay::type>::value && std::is_constructible::value && - !std::is_convertible::value, bool>::type = false> + !std::is_convertible::value, bool> = false> explicit optional(U&& v) : _engaged(true) { _storage.Init(std::forward(v)); } @@ -245,11 +245,11 @@ class optional { optional& operator=(optional&& rhs) = default; // Value assignment operators - template , typename std::decay::type>::value && !std::is_same, typename remove_cvref::type>::value && std::is_constructible::value && std::is_assignable::value && - (!std::is_scalar::value || !std::is_same::type>::value)>::type> + (!std::is_scalar::value || !std::is_same::type>::value)>> optional& operator=(U&& v) { reset(); _storage.Init(std::forward(v)); @@ -257,11 +257,11 @@ class optional { return *this; } - template ::value && !internal::is_constructible_convertible_assignable_from_optional::value && std::is_constructible::value && - std::is_assignable::value>::type> + std::is_assignable::value>> optional& operator=(const optional& rhs) { if (rhs) { operator=(*rhs); @@ -271,11 +271,11 @@ class optional { return *this; } - template ::value && !internal::is_constructible_convertible_assignable_from_optional::value && std::is_constructible::value && - std::is_assignable::value>::type> + std::is_assignable::value>> optional& operator=(optional&& rhs) { if (rhs) { operator=(std::move(*rhs)); diff --git a/src/butil/memory/scope_guard.h b/src/butil/memory/scope_guard.h index bd52b1487f..27fc2ba6de 100644 --- a/src/butil/memory/scope_guard.h +++ b/src/butil/memory/scope_guard.h @@ -24,7 +24,7 @@ namespace butil { template::value>::type> + typename = std::enable_if_t::value>> class ScopeGuard; template diff --git a/src/bvar/reducer.h b/src/bvar/reducer.h index 5b5afbd612..b9427edab1 100644 --- a/src/bvar/reducer.h +++ b/src/bvar/reducer.h @@ -73,10 +73,10 @@ struct IsBabylonCounterSupported // Selects the babylon-backed partial specializations of Adder/Maxer/Miner below. // NOTE: resolving to the *type* is a MUST. std::enable_if itself is a type // no matter what `cond` is, and Adder means Adder, so specializing on -// std::enable_if instead of its ::type silently never matches. +// std::enable_if instead of std::enable_if_t silently never matches. template using EnableIfBabylonCounter = - typename std::enable_if::value>::type; + std::enable_if_t::value>; template class BabylonVariable: public Variable { @@ -86,12 +86,12 @@ class BabylonVariable: public Variable { BabylonVariable() = default; - template::value, bool>::type = false> + template::value, bool> = false> BabylonVariable(U) {} // For Maxer. - template::value, bool>::type = false> + template::value, bool> = false> BabylonVariable(U default_value) : _counter(default_value) {} DISALLOW_COPY_AND_MOVE(BabylonVariable); From 2d2c5d8602242cb10da09d90e750ac2ac15e8063 Mon Sep 17 00:00:00 2001 From: chenBright Date: Fri, 18 Sep 2026 21:30:41 +0800 Subject: [PATCH 4/4] Opt BabylonVariable::reset() comment --- src/bvar/reducer.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/bvar/reducer.h b/src/bvar/reducer.h index b9427edab1..8b40fe3f81 100644 --- a/src/bvar/reducer.h +++ b/src/bvar/reducer.h @@ -131,8 +131,11 @@ class BabylonVariable: public Variable { T reset() { // Unlike AgentCombiner::reset_all_agents(), reading and clearing the babylon // counter are two separate steps, so values added in between are lost. This - // only affects explicit reset() by users: sampling of an operator without - // inverse is the only internal user and it runs in a single thread. + // affects both explicit reset() by users and periodic sampling by the sampler + // thread (e.g. Window/series sampling), where concurrent additions around each + // reset may be dropped. This is an accepted trade-off for the babylon backend: + // in statistics/monitoring scenarios such minor loss does not change the overall + // trend, so slightly inaccurate samples are acceptable. T result = _counter.value(); _counter.reset(); return result;