Copilot commented on code in PR #3009:
URL: https://github.com/apache/brpc/pull/3009#discussion_r2217172268
##########
src/bthread/task_group.cpp:
##########
@@ -82,6 +86,39 @@ void* run_create_span_func() {
return BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls).rpcz_parent_span;
}
+AtomicInteger128::Value AtomicInteger128::load() const {
+#if __x86_64__ || __ARM_NEON
+ // Supress compiler warning.
+ (void)_mutex;
+#endif // __x86_64__ || __ARM_NEON
+
+#if __x86_64__ || __ARM_NEON
+#ifdef __x86_64__
+ __m128i value = _mm_load_si128(reinterpret_cast<const __m128i*>(&_value));
+#else // __ARM_NEON
+ int64x2_t value = vld1q_s64(reinterpret_cast<const int64_t*>(&_value));
+#endif // __x86_64__
+ return {value[0], value[1]};
+#else // __x86_64__ || __ARM_NEON
+ BAIDU_SCOPED_LOCK(g->_mutex);
Review Comment:
The variable 'g' is undefined in this context. This should be '_mutex' since
we're inside the AtomicInteger128 class method.
```suggestion
BAIDU_SCOPED_LOCK(_mutex);
```
##########
src/bthread/task_group.cpp:
##########
@@ -176,31 +223,12 @@ void TaskGroup::run_main_task() {
}
}
// Don't forget to add elapse of last wait_task.
- current_task()->stat.cputime_ns += butil::cpuwide_time_ns() - _last_run_ns;
+ current_task()->stat.cputime_ns +=
+ butil::cpuwide_time_ns() -
std::abs(_cpu_time_stat.load_unsafe().last_run_ns());
Review Comment:
Using std::abs() on the result of last_run_ns() is incorrect. The method
last_run_ns() already masks out the sign bit and returns a positive value. This
suggests a misunderstanding of the bit packing format.
```suggestion
butil::cpuwide_time_ns() -
_cpu_time_stat.load_unsafe().last_run_ns();
```
##########
src/bthread/task_group.cpp:
##########
@@ -82,6 +86,39 @@ void* run_create_span_func() {
return BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls).rpcz_parent_span;
}
+AtomicInteger128::Value AtomicInteger128::load() const {
+#if __x86_64__ || __ARM_NEON
+ // Supress compiler warning.
+ (void)_mutex;
+#endif // __x86_64__ || __ARM_NEON
+
+#if __x86_64__ || __ARM_NEON
+#ifdef __x86_64__
+ __m128i value = _mm_load_si128(reinterpret_cast<const __m128i*>(&_value));
+#else // __ARM_NEON
+ int64x2_t value = vld1q_s64(reinterpret_cast<const int64_t*>(&_value));
+#endif // __x86_64__
+ return {value[0], value[1]};
+#else // __x86_64__ || __ARM_NEON
+ BAIDU_SCOPED_LOCK(g->_mutex);
+ return _value;
+#endif // __x86_64__ || __ARM_NEON
+}
+
+void AtomicInteger128::store(Value value) {
+#if __x86_64__
+ __m128i v = _mm_load_si128(reinterpret_cast<__m128i*>(&value));
+ _mm_store_si128(reinterpret_cast<__m128i*>(&_value), v);
+#elif __ARM_NEON
+ int64x2_t v = vld1q_s64(reinterpret_cast<int64_t*>(&value));
+ vst1q_s64(reinterpret_cast<int64_t*>(&_value), v);
+#else
+ BAIDU_SCOPED_LOCK(g->_mutex);
Review Comment:
The variable 'g' is undefined in this context. This should be '_mutex' since
we're inside the AtomicInteger128 class method.
```suggestion
BAIDU_SCOPED_LOCK(_mutex);
```
##########
src/bthread/task_group.h:
##########
@@ -203,6 +233,68 @@ class TaskGroup {
private:
friend class TaskControl;
+ // Last scheduling time, task type and cumulated CPU time.
+ class CPUTimeStat {
+ public:
+ CPUTimeStat() : _last_run_ns_and_type(0), _cumulated_cputime_ns(0) {}
+ CPUTimeStat(AtomicInteger128::Value value)
+ : _last_run_ns_and_type(value.v1), _cumulated_cputime_ns(value.v2)
{}
+
+ explicit operator AtomicInteger128::Value() const {
+ return {_last_run_ns_and_type, _cumulated_cputime_ns};
+ }
+
+ void set_last_run_ns(int64_t last_run_ns, bool main_task) {
+ _last_run_ns_and_type = main_task;
+ _last_run_ns_and_type <<= 63;
+ _last_run_ns_and_type |= last_run_ns & 0x7FFFFFFFFFFFFFFFLL;
+ }
+ int64_t last_run_ns() const {
+ return _last_run_ns_and_type & 0x7FFFFFFFFFFFFFFFLL;
Review Comment:
The bit manipulation logic in this method uses magic numbers (63,
0x7FFFFFFFFFFFFFFFLL). Consider defining named constants for these values to
improve readability and maintainability.
```suggestion
_last_run_ns_and_type <<= TASK_TYPE_BIT_POSITION;
_last_run_ns_and_type |= last_run_ns & LAST_SCHEDULING_TIME_MASK;
}
int64_t last_run_ns() const {
return _last_run_ns_and_type & LAST_SCHEDULING_TIME_MASK;
```
--
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-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
