You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "chenBright (via GitHub)" <gi...@apache.org> on 2026/09/16 01:59:10 UTC
[PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
chenBright opened a new pull request, #3547:
URL: https://github.com/apache/brpc/pull/3547
### What problem does this PR solve?
Issue Number: resolve #3546
Problem Summary:
`std::enable_if<cond>` is a class type whatever `cond` is, only `std::enable_if<cond>::type`
goes away when the condition fails. Three places write the former, so their constraints check
nothing.
- `bvar/reducer.h`: the babylon backed partial specializations of `Adder`/`Maxer`/`Miner` are
keyed on `std::enable_if<cond>` while `Adder<T>` means `Adder<T, void>`, so they never
match and the babylon counters are dead code even when built with
`--define with_babylon_counter=true`. Adding the missing `::type` is not enough: babylon
constrains its counters with a `static_assert` in the class body, so probing them with
`std::is_constructible` is a hard error instead of a substitution failure, and the specializations
then expose three latent defects. `Adder::reset()` hits an inverted `CHECK` and never resets,
`Maxer`/`Miner` return 0 instead of the identity of their operator for an empty sampling period,
and `get_value()` carries no `CHECK` so `Window<Maxer<> >` loses its warning.
- `butil/memory/scope_guard.h`: the default argument of the primary template and the argument
list of the partial specialization share the same wrong expression, so they always agree and a
callback returning non void is silently accepted.
- `butil/containers/optional.h`: the in place constructor is constrained by `std::enable_if<cond>*`,
a valid pointer type whatever `cond` is, so `optional<int>` is reported constructible from a string
literal. The `initializer_list` in place constructor puts its constrained parameter after a parameter
pack without a default argument, so it can neither be deduced nor specified and is never viable.
### What is changed and the side effects?
Changed:
`bvar/reducer.h`
- Key the babylon backed specializations on `typename std::enable_if<cond>::type`.
- Replace the `std::is_constructible` probe with `IsBabylonCounterSupported`, a two step trait
mirroring the constraint of babylon counters. It cannot be one expression because `sizeof` does
not apply to void or to an incomplete type.
`butil/memory/scope_guard.h`
- Resolve the constraint to `void` and specialize on `ScopeGuard<Callback, void>`.
`butil/containers/optional.h`
- Constrain both in place constructors with `typename std::enable_if<cond, bool>::type = false`,
the form already used elsewhere in the file.
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4045623202
##########
src/bvar/reducer.h:
##########
@@ -93,17 +121,18 @@ class BabylonVariable: public Variable {
}
T get_value() const {
+ CHECK(!(butil::is_same<InvOp, VoidOp>::value) || nullptr == _sampler)
+ << "You should not call Reducer<" << butil::class_name_str<T>()
+ << ", " << butil::class_name_str<Op>() << ">::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<VoidOp, InvOp>::value))) {
- CHECK(false) << "You should not call Reducer<" << butil::class_name_str<T>()
- << ", " << butil::class_name_str<Op>() << ">::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.
Review Comment:
> If this is an accepted trade-off for the Babylon backend, could we update the comment to acknowledge that it > also applies to normal sampling?
@wasphin You are right. Updated the comment to explicitly state that this affects sampler-thread sampling (e.g. Window/series sampling) as well, and that it's an accepted trade-off for the babylon backend since minor loss doesn't change the overall trend in statistics/monitoring scenarios.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4022015641
##########
src/bvar/reducer.h:
##########
@@ -93,17 +121,18 @@ class BabylonVariable: public Variable {
}
T get_value() const {
+ CHECK(!(butil::is_same<InvOp, VoidOp>::value) || nullptr == _sampler)
+ << "You should not call Reducer<" << butil::class_name_str<T>()
+ << ", " << butil::class_name_str<Op>() << ">::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<VoidOp, InvOp>::value))) {
- CHECK(false) << "You should not call Reducer<" << butil::class_name_str<T>()
- << ", " << butil::class_name_str<Op>() << ">::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.
Review Comment:
This is a design limitation of the babylon counters, expected and acceptable here.
babylon offers no atomic read-and-clear: `value()` aggregates the thread-local
slots and `reset()` only bumps a version counter. babylon documents the very same
window on the write side (`counter.h`, `operator<<`): a sample landing across a
version bump is attributed to the previous period and dropped, and the author
explicitly deems it negligible for statistics. Providing a snapshot-and-clear path
would mean synchronizing every `operator<<`, which is the cost the babylon backend
exists to avoid.
The only internal caller is `ReducerSampler::take_sample()`, which for an operator
without inverse runs in the single sampling thread, so sampling is unaffected. Only
an explicit user `reset()` racing with `operator<<` can lose a sample, which is
what the comment at that line states.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "lgqss (via GitHub)" <gi...@apache.org>.
lgqss commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4023352825
##########
src/butil/containers/optional.h:
##########
@@ -145,45 +145,45 @@ class optional {
optional(optional&& rhs) noexcept = default;
- template <typename U, typename std::enable_if<
+ template <typename U, std::enable_if_t<
Review Comment:
enable_if_t 后续不再支持c++11编译了吗
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "wasphin (via GitHub)" <gi...@apache.org>.
wasphin commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4037643838
##########
src/bvar/reducer.h:
##########
@@ -93,17 +121,18 @@ class BabylonVariable: public Variable {
}
T get_value() const {
+ CHECK(!(butil::is_same<InvOp, VoidOp>::value) || nullptr == _sampler)
+ << "You should not call Reducer<" << butil::class_name_str<T>()
+ << ", " << butil::class_name_str<Op>() << ">::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<VoidOp, InvOp>::value))) {
- CHECK(false) << "You should not call Reducer<" << butil::class_name_str<T>()
- << ", " << butil::class_name_str<Op>() << ">::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.
Review Comment:
Could you clarify the comment about sampling being unaffected? Although `ReducerSampler::take_sample()` runs in a single thread, writer threads can still update their thread-local slots between `value()` and `reset()`, so normal sampling appears able to lose updates too.
If this is an accepted trade-off for the Babylon backend, could we update the comment to acknowledge that it also applies to normal sampling?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4021946956
##########
src/bvar/reducer.h:
##########
@@ -449,28 +476,25 @@ class ConcurrentMaxer : public babylon::GenericsConcurrentMaxer<T> {
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<T>::min()};
};
} // namespace detail
// Numerical types supported by babylon counter.
template <typename T>
-class Maxer<T, std::enable_if<std::is_constructible<detail::ConcurrentMaxer<T>>::value>>
+class Maxer<T, detail::EnableIfBabylonCounter<T>>
Review Comment:
This constraint also selects the Babylon implementation for `Maxer<double>`, but Babylon 1.4.4 initializes its max comparator with `std::numeric_limits<T>::min()`. For floating-point `T`, that is the smallest positive value, so a sample such as `-1.0` is ignored and the reducer returns the positive sentinel instead of the maximum. Restrict the Babylon Maxer path to integral types or change the underlying/generic max identity to `numeric_limits<T>::lowest()`.
##########
src/bvar/reducer.h:
##########
@@ -93,17 +121,18 @@ class BabylonVariable: public Variable {
}
T get_value() const {
+ CHECK(!(butil::is_same<InvOp, VoidOp>::value) || nullptr == _sampler)
+ << "You should not call Reducer<" << butil::class_name_str<T>()
+ << ", " << butil::class_name_str<Op>() << ">::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<VoidOp, InvOp>::value))) {
- CHECK(false) << "You should not call Reducer<" << butil::class_name_str<T>()
- << ", " << butil::class_name_str<Op>() << ">::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.
Review Comment:
Because `value()` and `reset()` are separate operations, an `operator<<` concurrent with this path can be included in the returned value and then erased, or added after the read and lost by `reset()`. `Reducer::reset()` otherwise delegates to `AgentCombiner::reset_all_agents()`, which exchanges values under its lock. Please provide an atomic snapshot-and-clear path (or synchronize updates with reset) rather than accepting lost updates.
##########
src/butil/containers/optional.h:
##########
@@ -199,14 +199,15 @@ class optional {
_storage.Init(std::move(value));
}
- template <typename... Args,
- std::enable_if<std::is_constructible<T, Args&&...>::value>* = nullptr>
- explicit optional(const in_place_t, Args&&... args) : _engaged(true) {
+ template <typename... Args, typename std::enable_if<
+ std::is_constructible<T, Args&&...>::value, bool>::type = false>
+ explicit optional(in_place_t, Args&&... args) : _engaged(true) {
Review Comment:
These constraints change compile-time overload selection, but the existing optional tests do not exercise either in-place constructor. Add a negative constructibility assertion for `optional<int>` with `in_place_t` and a string argument, plus a positive initializer-list in-place construction, so regressions in the corrected `enable_if` and the newly viable initializer-list overload are caught.
##########
src/butil/memory/scope_guard.h:
##########
@@ -24,7 +24,7 @@
namespace butil {
template<typename Callback,
- typename = std::enable_if<is_result_void<Callback>::value>>
+ typename = typename std::enable_if<is_result_void<Callback>::value>::type>
Review Comment:
The existing scope-guard tests only instantiate void-returning callbacks, so they would also pass with the old unconstrained `std::enable_if<...>` class argument. Add a compile-time detection test for a non-void callback to lock in that `MakeScopeGuard` is rejected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "wasphin (via GitHub)" <gi...@apache.org>.
wasphin merged PR #3547:
URL: https://github.com/apache/brpc/pull/3547
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4022137655
##########
src/bvar/reducer.h:
##########
@@ -93,17 +121,18 @@ class BabylonVariable: public Variable {
}
T get_value() const {
+ CHECK(!(butil::is_same<InvOp, VoidOp>::value) || nullptr == _sampler)
+ << "You should not call Reducer<" << butil::class_name_str<T>()
+ << ", " << butil::class_name_str<Op>() << ">::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<VoidOp, InvOp>::value))) {
- CHECK(false) << "You should not call Reducer<" << butil::class_name_str<T>()
- << ", " << butil::class_name_str<Op>() << ">::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;
Review Comment:
The new Babylon reset path is not exercised: the reducer tests cover `Adder::get_value()` but never assert that `reset()` returns the accumulated value and restores the identity. This path replaces the previous inverted CHECK, so add a test that runs with `WITH_BABYLON_COUNTER` enabled as well as with the generic backend.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "wasphin (via GitHub)" <gi...@apache.org>.
wasphin commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4023001264
##########
src/butil/containers/optional.h:
##########
@@ -199,14 +199,15 @@ class optional {
_storage.Init(std::move(value));
}
- template <typename... Args,
- std::enable_if<std::is_constructible<T, Args&&...>::value>* = nullptr>
- explicit optional(const in_place_t, Args&&... args) : _engaged(true) {
+ template <typename... Args, typename std::enable_if<
+ std::is_constructible<T, Args&&...>::value, bool>::type = false>
Review Comment:
something like bellow in C++14 is more clear
```
template <typename... Args,
std::enable_if_t<std::is_constructible<T, Args&&...>::value, int> = 0>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4023466108
##########
src/butil/containers/optional.h:
##########
@@ -145,45 +145,45 @@ class optional {
optional(optional&& rhs) noexcept = default;
- template <typename U, typename std::enable_if<
+ template <typename U, std::enable_if_t<
Review Comment:
对，#3419 升到 C++14 了。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4045623202
##########
src/bvar/reducer.h:
##########
@@ -93,17 +121,18 @@ class BabylonVariable: public Variable {
}
T get_value() const {
+ CHECK(!(butil::is_same<InvOp, VoidOp>::value) || nullptr == _sampler)
+ << "You should not call Reducer<" << butil::class_name_str<T>()
+ << ", " << butil::class_name_str<Op>() << ">::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<VoidOp, InvOp>::value))) {
- CHECK(false) << "You should not call Reducer<" << butil::class_name_str<T>()
- << ", " << butil::class_name_str<Op>() << ">::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.
Review Comment:
> If this is an accepted trade-off for the Babylon backend, could we update the comment to acknowledge that it > also applies to normal sampling?
@wasphin You are right. Updated the comment in 2d2c5d8602242cb10da09d90e750ac2ac15e8063 to explicitly state that this affects sampler-thread sampling (e.g. Window/series sampling) as well, and that it's an accepted trade-off for the babylon backend since minor loss doesn't change the overall trend in statistics/monitoring scenarios.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4023175384
##########
src/butil/containers/optional.h:
##########
@@ -199,14 +199,15 @@ class optional {
_storage.Init(std::move(value));
}
- template <typename... Args,
- std::enable_if<std::is_constructible<T, Args&&...>::value>* = nullptr>
- explicit optional(const in_place_t, Args&&... args) : _engaged(true) {
+ template <typename... Args, typename std::enable_if<
+ std::is_constructible<T, Args&&...>::value, bool>::type = false>
Review Comment:
Done in b5a83bf0a5d592083a9450f6afdb8dcd0a809b0f .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4022020844
##########
src/bvar/reducer.h:
##########
@@ -449,28 +476,25 @@ class ConcurrentMaxer : public babylon::GenericsConcurrentMaxer<T> {
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<T>::min()};
};
} // namespace detail
// Numerical types supported by babylon counter.
template <typename T>
-class Maxer<T, std::enable_if<std::is_constructible<detail::ConcurrentMaxer<T>>::value>>
+class Maxer<T, detail::EnableIfBabylonCounter<T>>
Review Comment:
`Maxer<double>` already behaves exactly like this without the babylon backend: the
generic `Maxer` initializes its identity with `std::numeric_limits<T>::min()`
(`reducer.h:442`), so `MaxTo` drops `-1.0` and `get_value()` returns the positive
sentinel. `detail::ConcurrentMaxer` mirrors that identity on purpose, so both
implementations return the same value for the same input, which is the property
this PR is about. babylon's comparer behaves consistently with it: it initializes
`result` to `EXTREMUM = numeric_limits<T>::min()` and reports no result when every
sample compares below it, at which point the wrapper returns the identity.
Restricting the babylon path to integral types would make the two implementations
disagree instead of removing the sentinel. Switching to `numeric_limits<T>::lowest()`
is the right fix, but it changes the long-standing behavior of the generic
`Maxer<double>` as well, so I would rather do it in a separate PR that fixes both
paths and adds a floating point test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4022239198
##########
src/bvar/reducer.h:
##########
@@ -93,17 +121,18 @@ class BabylonVariable: public Variable {
}
T get_value() const {
+ CHECK(!(butil::is_same<InvOp, VoidOp>::value) || nullptr == _sampler)
+ << "You should not call Reducer<" << butil::class_name_str<T>()
+ << ", " << butil::class_name_str<Op>() << ">::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<VoidOp, InvOp>::value))) {
- CHECK(false) << "You should not call Reducer<" << butil::class_name_str<T>()
- << ", " << butil::class_name_str<Op>() << ">::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;
Review Comment:
Done in 8f3ec497d1c9b74d0d3ca40c18e59d4745b135fd .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix ineffective enable_if constraints in bvar and butil (brpc)
Posted by "wasphin (via GitHub)" <gi...@apache.org>.
wasphin commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4037643838
##########
src/bvar/reducer.h:
##########
@@ -93,17 +121,18 @@ class BabylonVariable: public Variable {
}
T get_value() const {
+ CHECK(!(butil::is_same<InvOp, VoidOp>::value) || nullptr == _sampler)
+ << "You should not call Reducer<" << butil::class_name_str<T>()
+ << ", " << butil::class_name_str<Op>() << ">::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<VoidOp, InvOp>::value))) {
- CHECK(false) << "You should not call Reducer<" << butil::class_name_str<T>()
- << ", " << butil::class_name_str<Op>() << ">::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.
Review Comment:
@chenBright Could you clarify the comment about sampling being unaffected? Although `ReducerSampler::take_sample()` runs in a single thread, writer threads can still update their thread-local slots between `value()` and `reset()`, so normal sampling appears able to lose updates too.
If this is an accepted trade-off for the Babylon backend, could we update the comment to acknowledge that it also applies to normal sampling?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org