Copilot commented on code in PR #3603:
URL: https://github.com/apache/brpc/pull/3603#discussion_r4266162520
##########
src/bthread/timer_thread.cpp:
##########
@@ -298,13 +295,15 @@ int TimerThread::unschedule(TaskId task_id) {
return (expected_version == id_version + 1) ? 1 : -1;
}
-bool TimerThread::Task::run_and_delete() {
+bool TimerThread::Task::run_and_delete(bvar::LatencyRecorder& task_latency) {
const uint32_t id_version = version_of_task_id(task_id);
uint32_t expected_version = id_version;
// This CAS is rarely contended, should be fast.
- if (version.compare_exchange_strong(
- expected_version, id_version + 1, butil::memory_order_relaxed)) {
+ if (version.compare_exchange_strong(expected_version, id_version + 1,
+ butil::memory_order_relaxed)) {
+ int64_t start_us = butil::cpuwide_time_us();
fn(arg);
+ task_latency << butil::cpuwide_time_us() - start_us;
Review Comment:
Latency measurement + recorder update is now executed for every task run,
even when `_options.bvar_prefix` is empty (i.e., the bvar is never exposed).
This adds overhead on the hot path. Consider gating the timing/recording behind
the same condition used for exposure (e.g., only record when monitoring is
enabled), or pass an optional pointer/flag so `run_and_delete` can skip the
timing work when not needed.
##########
src/bthread/timer_thread.cpp:
##########
@@ -71,24 +70,22 @@ TimerThreadOptions::TimerThreadOptions()
// A task contains the necessary information for running fn(arg).
// Tasks are created in Bucket::schedule and destroyed in TimerThread::run
struct BAIDU_CACHELINE_ALIGNMENT TimerThread::Task {
- Task* next; // For linking tasks in a Bucket.
- int64_t run_time; // run the task at this realtime
- void (*fn)(void*); // the fn(arg) to run
- void* arg;
+ Task* next{NULL}; // For linking tasks in a Bucket.
+ int64_t run_time{0}; // run the task at this realtime
+ void (*fn)(void*){NULL}; // the fn(arg) to run
+ void* arg{NULL};
// Current TaskId, checked against version in TimerThread::run to test
// if this task is unscheduled.
- TaskId task_id;
+ TaskId task_id{0};
// initial_version: not run yet
// initial_version + 1: running
// initial_version + 2: removed (also the version of next Task reused
// this struct)
- butil::atomic<uint32_t> version;
-
- Task() : version(2/*skip 0*/) {}
+ butil::atomic<uint32_t> version{2/*skip 0*/};
// Run this task and delete this struct.
// Returns true if fn(arg) did run.
Review Comment:
The `run_and_delete` comment/doc no longer matches the signature/behavior
now that it records into `task_latency`. Please update the comment to document
that latency is recorded (and the unit, e.g., microseconds) and clarify whether
it records only when `fn(arg)` actually runs.
##########
src/bthread/timer_thread.cpp:
##########
@@ -71,24 +70,22 @@ TimerThreadOptions::TimerThreadOptions()
// A task contains the necessary information for running fn(arg).
// Tasks are created in Bucket::schedule and destroyed in TimerThread::run
struct BAIDU_CACHELINE_ALIGNMENT TimerThread::Task {
- Task* next; // For linking tasks in a Bucket.
- int64_t run_time; // run the task at this realtime
- void (*fn)(void*); // the fn(arg) to run
- void* arg;
+ Task* next{NULL}; // For linking tasks in a Bucket.
+ int64_t run_time{0}; // run the task at this realtime
+ void (*fn)(void*){NULL}; // the fn(arg) to run
+ void* arg{NULL};
Review Comment:
New in-class initializers use `NULL` for pointer and function-pointer
fields. In C++11+ code, `nullptr` is type-safe and avoids overload ambiguity.
Consider switching these initializers to `nullptr` for consistency and safety.
##########
src/bthread/timer_thread.cpp:
##########
@@ -365,10 +364,12 @@ void TimerThread::run() {
double busy_seconds = 0;
bvar::PassiveStatus<double> busy_seconds_var(deref_value<double>,
&busy_seconds);
bvar::PerSecond<bvar::PassiveStatus<double> >
busy_seconds_second(&busy_seconds_var);
+ bvar::LatencyRecorder task_latency;
if (!_options.bvar_prefix.empty()) {
nscheduled_second.expose_as(_options.bvar_prefix, "scheduled_second");
ntriggered_second.expose_as(_options.bvar_prefix, "triggered_second");
busy_seconds_second.expose_as(_options.bvar_prefix, "usage");
+ task_latency.expose(_options.bvar_prefix, "task");
Review Comment:
The exposed bvar name `\"task\"` is very generic and may be
ambiguous/collision-prone under a shared prefix (it also doesn’t indicate that
this is latency). Consider renaming to something more specific like
`task_latency` / `task_us` / `task_latency_us` to better communicate what is
being recorded.
--
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]
