Copilot commented on code in PR #3116:
URL: https://github.com/apache/brpc/pull/3116#discussion_r2425113695
##########
src/bvar/reducer.h:
##########
@@ -283,41 +421,112 @@ class Maxer : public Reducer<T, detail::MaxTo<T> > {
}
};
+#if WITH_BABYLON_COUNTER
+namespace detail {
+template <typename T>
+class ConcurrentMaxer : public babylon::GenericsConcurrentMaxer<T> {
+ typedef babylon::GenericsConcurrentMaxer<T> Base;
+public:
+ ConcurrentMaxer() = default;
+ ConcurrentMaxer(T default_value) : _default_value(default_value) {}
+
+ T value() const {
+ T result;
+ if (!Base::value(result)) {
+ return _default_value;
+ }
+ return std::max(result, _default_value);
+ }
+private:
+ T _default_value{0};
+};
+} // namespace detail
+
+// Numerical types supported by babylon counter.
+template <typename T>
+class Maxer<T,
std::enable_if<std::is_constructible<detail::ConcurrentMaxer<T>>::value>>
Review Comment:
The template specialization is missing the second template parameter type.
Should be
`std::enable_if<std::is_constructible<detail::ConcurrentMaxer<T>>::value>::type`.
```suggestion
class Maxer<T, typename
std::enable_if<std::is_constructible<detail::ConcurrentMaxer<T>>::value>::type>
```
##########
src/bvar/recorder.h:
##########
@@ -286,6 +294,100 @@ inline IntRecorder& IntRecorder::operator<<(int64_t
sample) {
n, _compress(num + 1, sum + complement)));
return *this;
}
+#else // WITH_BABYLON_COUNTER
+class IntRecorder : public Variable {
+public:
+ typedef Stat value_type;
+ typedef detail::AddStat Op;
+ typedef detail::MinusStat InvOp;
+ typedef detail::ReducerSampler<IntRecorder, value_type, Op, InvOp>
sampler_type;
+
+ COMMON_VARIABLE_CONSTRUCTOR(IntRecorder);
+
+ DISALLOW_COPY_AND_MOVE(IntRecorder);
+
+ ~IntRecorder() override {
+ hide();
+ if (NULL != _sampler) {
+ _sampler->destroy();
+ }
+ }
+
+ // Note: The input type is acutally int. Use int64_t to check overflow.
+ IntRecorder& operator<<(int64_t value) {
+ if (BAIDU_UNLIKELY((int64_t)(int)value != value)) {
+ const char* reason = NULL;
+ if (value > std::numeric_limits<int>::max()) {
+ reason = "overflows";
+ value = std::numeric_limits<int>::max();
+ } else {
+ reason = "underflows";
+ value = std::numeric_limits<int>::min();
+ }
+ // Truncate to be max or min of int. We're using 44 bits to store
the
+ // sum thus following aggregations are not likely to be
over/underflow.
+ if (!name().empty()) {
+ LOG(WARNING) << "Input=" << value << " to `" << name()
+ << "\' " << reason;
+ } else if (!_debug_name.empty()) {
+ LOG(WARNING) << "Input=" << value << " to `" << _debug_name
+ << "\' " << reason;
+ } else {
+ LOG(WARNING) << "Input=" << value << " to IntRecorder("
+ << (void*)this << ") " << reason;
+ }
+ }
+
+ _summer << value;
+ return *this;
+ }
+
+ int64_t average() const {
+ return get_value().get_average_int();
+ }
+
+ double average(double) const {
+ return get_value().get_average_double();
+ }
+
+ value_type get_value() const {
+ auto summary = _summer.value();
+ return value_type{summary.sum, static_cast<ssize_t>(summary.num)};
+ }
+
+ value_type reset() {
+ LOG_EVERY_SECOND(ERROR) << "IntRecorder with babylon counter should
never call reset()";
Review Comment:
Error message should be more descriptive about why reset() cannot be called
and what alternative should be used.
```suggestion
LOG_EVERY_SECOND(ERROR) << "IntRecorder with babylon counter does
not support reset() because concurrent aggregation cannot be reset safely. If
you need reset functionality, use a different recorder type (e.g., the default
IntRecorder) that supports reset().";
```
##########
src/bvar/detail/percentile.h:
##########
@@ -499,6 +552,56 @@ class Percentile {
sampler_type* _sampler;
std::string _debug_name;
};
+#else
+class Percentile {
+public:
+ typedef GlobalPercentileSamples value_type;
+ typedef detail::AddPercentileSamples AddPercentileSamples;
+ typedef AddPercentileSamples Op;
+ typedef VoidOp InvOp;
+ typedef ReducerSampler<Percentile, value_type, Op, InvOp> sampler_type;
+
+ Percentile() = default;
+ DISALLOW_COPY_AND_MOVE(Percentile);
+ ~Percentile() noexcept {
+ if (NULL != _sampler) {
+ _sampler->destroy();
+ }
+ }
+
+ Op op() const { return Op(); }
+ InvOp inv_op() const { return InvOp(); }
+
+ sampler_type* get_sampler() {
+ if (NULL == _sampler) {
+ _sampler = new sampler_type(this);
+ _sampler->schedule();
+ }
+ return _sampler;
+ }
+
+ value_type reset();
+
+ value_type get_value() const {
+ LOG_EVERY_SECOND(ERROR) << "Percentile should never call this
get_value()";
Review Comment:
Error message should be more descriptive about why get_value() cannot be
called and what alternative should be used.
```suggestion
LOG_EVERY_SECOND(ERROR) << "Percentile::get_value() cannot be called
directly because Percentile does not support direct value retrieval. Please use
get_sampler()->get_value() to obtain the value instead.";
```
##########
src/bvar/reducer.h:
##########
@@ -283,41 +421,112 @@ class Maxer : public Reducer<T, detail::MaxTo<T> > {
}
};
+#if WITH_BABYLON_COUNTER
+namespace detail {
+template <typename T>
+class ConcurrentMaxer : public babylon::GenericsConcurrentMaxer<T> {
+ typedef babylon::GenericsConcurrentMaxer<T> Base;
+public:
+ ConcurrentMaxer() = default;
+ ConcurrentMaxer(T default_value) : _default_value(default_value) {}
+
+ T value() const {
+ T result;
+ if (!Base::value(result)) {
+ return _default_value;
+ }
+ return std::max(result, _default_value);
+ }
+private:
+ T _default_value{0};
+};
+} // namespace detail
+
+// Numerical types supported by babylon counter.
+template <typename T>
+class Maxer<T,
std::enable_if<std::is_constructible<detail::ConcurrentMaxer<T>>::value>>
+ : public detail::BabylonVariable<T, detail::ConcurrentMaxer<T>,
+ detail::MaxTo<T>, detail::VoidOp> {
+public:
+ typedef T value_type;
+private:
+ typedef detail::BabylonVariable<T, detail::ConcurrentMaxer<T>,
+ detail::MaxTo<value_type>, detail::VoidOp>
Base;
+public:
+ typedef detail::MaxTo<value_type> Op;
+ typedef detail::VoidOp InvOp;
+ typedef typename Base::sampler_type sampler_type;
+
+ COMMON_VARIABLE_CONSTRUCTOR(Maxer);
+
+private:
+friend class detail::LatencyRecorderBase;
+
+ Maxer(T default_value) : Base(default_value) {}
+ Maxer(T default_value, const butil::StringPiece& prefix, const
butil::StringPiece& name)
+ : Base(default_value) {
+ Variable::expose_as(prefix, name);
+ }
+ Maxer(T default_value, const butil::StringPiece& name)
+ : Base(default_value) {
+ Variable::expose(name);
+ }
+};
+#endif // WITH_BABYLON_COUNTER
+
// bvar::Miner<int> min_value;
// min_value << 1 << 2 << 3 << 4;
// LOG(INFO) << min_value.get_value(); // 1
namespace detail {
-
-template <typename Tp>
+template <typename Tp>
struct MinTo {
- void operator()(Tp & lhs,
+ void operator()(Tp & lhs,
typename butil::add_cr_non_integral<Tp>::type rhs) const {
if (rhs < lhs) {
lhs = rhs;
}
}
};
-
} // namespace detail
-template <typename T>
+template <typename T, typename = void>
class Miner : public Reducer<T, detail::MinTo<T> > {
public:
typedef Reducer<T, detail::MinTo<T> > Base;
typedef T value_type;
typedef typename Base::sampler_type sampler_type;
-public:
+
Miner() : Base(std::numeric_limits<T>::max()) {}
- explicit Miner(const butil::StringPiece& name)
+ Miner(const butil::StringPiece& name)
: Base(std::numeric_limits<T>::max()) {
this->expose(name);
}
Miner(const butil::StringPiece& prefix, const butil::StringPiece& name)
: Base(std::numeric_limits<T>::max()) {
this->expose_as(prefix, name);
}
- ~Miner() { Variable::hide(); }
+ ~Miner() override { Variable::hide(); }
+};
+
+#if WITH_BABYLON_COUNTER
+// Numerical types supported by babylon counter.
+template <typename T>
+class Miner<T,
std::enable_if<std::is_constructible<babylon::GenericsConcurrentMiner<T>>::value>>
Review Comment:
The template specialization is missing the second template parameter type.
Should be
`std::enable_if<std::is_constructible<babylon::GenericsConcurrentMiner<T>>::value>::type`.
```suggestion
class Miner<T, typename
std::enable_if<std::is_constructible<babylon::GenericsConcurrentMiner<T>>::value>::type>
```
##########
src/bvar/reducer.h:
##########
@@ -29,9 +29,133 @@
#include "bvar/detail/sampler.h" // ReducerSampler
#include "bvar/detail/series.h"
#include "bvar/window.h"
+#if WITH_BABYLON_COUNTER
+#include "babylon/concurrent/counter.h"
+#endif // WITH_BABYLON_COUNTER
namespace bvar {
+namespace detail {
+template<typename O, typename T, typename Op>
+class SeriesSamplerImpl : public Sampler {
+public:
+ SeriesSamplerImpl(O* owner, const Op& op)
+ : _owner(owner), _series(op) {}
+ void take_sample() override { _series.append(_owner->get_value()); }
+ void describe(std::ostream& os) { _series.describe(os, NULL); }
+
+private:
+ O* _owner;
+ Series<T, Op> _series;
+};
+
+#if WITH_BABYLON_COUNTER
+template<typename T, typename Counter, typename Op, typename InvOp>
+class BabylonVariable: public Variable {
+public:
+ typedef ReducerSampler<BabylonVariable, T, Op, InvOp> sampler_type;
+ typedef SeriesSamplerImpl<BabylonVariable, T, Op> series_sampler_type;
+
+ BabylonVariable() = default;
+
+ template<typename U = T, typename std::enable_if<
+ !std::is_constructible<Counter, U>::value, bool>::type = false>
+ BabylonVariable(U) {}
+ // For Maxer.
+ template<typename U = T, typename std::enable_if<
+ std::is_constructible<Counter, U>::value, bool>::type = false>
+ BabylonVariable(U default_value) : _counter(default_value) {}
+
+ DISALLOW_COPY_AND_MOVE(BabylonVariable);
+
+ ~BabylonVariable() override {
+ hide();
+ if (NULL!= _sampler) {
Review Comment:
Missing space after 'NULL' before '!=' operator.
##########
.github/pull_request_template.md:
##########
@@ -1,6 +1,6 @@
### What problem does this PR solve?
-Issue Number:
+Issue Number: resolve
Review Comment:
The template change appears incomplete - 'resolve' should be followed by the
issue number or removed if this was unintentional.
```suggestion
Issue Number: <!-- e.g. #1234, or N/A -->
```
##########
src/bvar/reducer.h:
##########
@@ -205,34 +319,55 @@ inline Reducer<T, Op, InvOp>& Reducer<T, Op,
InvOp>::operator<<(
namespace detail {
template <typename Tp>
struct AddTo {
- void operator()(Tp & lhs,
+ void operator()(Tp & lhs,
typename butil::add_cr_non_integral<Tp>::type rhs) const
{ lhs += rhs; }
};
template <typename Tp>
struct MinusFrom {
- void operator()(Tp & lhs,
+ void operator()(Tp & lhs,
typename butil::add_cr_non_integral<Tp>::type rhs) const
{ lhs -= rhs; }
};
-}
-template <typename T>
+} // namespace detail
+
+template <typename T, typename = void>
class Adder : public Reducer<T, detail::AddTo<T>, detail::MinusFrom<T> > {
public:
typedef Reducer<T, detail::AddTo<T>, detail::MinusFrom<T> > Base;
typedef T value_type;
typedef typename Base::sampler_type sampler_type;
-public:
+
Adder() : Base() {}
- explicit Adder(const butil::StringPiece& name) : Base() {
+ Adder(const butil::StringPiece& name) : Base() {
this->expose(name);
}
Adder(const butil::StringPiece& prefix,
const butil::StringPiece& name) : Base() {
this->expose_as(prefix, name);
}
- ~Adder() { Variable::hide(); }
+ ~Adder() override { Variable::hide(); }
+};
+
+#if WITH_BABYLON_COUNTER
+// Numerical types supported by babylon counter.
+template <typename T>
+class Adder<T,
std::enable_if<std::is_constructible<babylon::GenericsConcurrentAdder<T>>::value>>
Review Comment:
The template specialization is missing the second template parameter type.
Should be
`std::enable_if<std::is_constructible<babylon::GenericsConcurrentAdder<T>>::value>::type`.
```suggestion
class Adder<T, typename
std::enable_if<std::is_constructible<babylon::GenericsConcurrentAdder<T>>::value>::type>
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
