Copilot commented on code in PR #3603:
URL: https://github.com/apache/brpc/pull/3603#discussion_r4266517643
##########
src/bthread/timer_thread.cpp:
##########
@@ -298,13 +296,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 is recorded for every triggered task even when
`_options.bvar_prefix` is empty (i.e., the variable is never exposed/used).
This adds avoidable overhead (extra time reads + recorder updates) to a hot
path. Consider gating the timing + `task_latency << ...` behind a boolean that
is enabled only when the LatencyRecorder is exposed (e.g.,
`_options.bvar_prefix` non-empty), or pass an optional recorder
(pointer/nullable) so the task can skip measurement entirely when monitoring is
disabled.
##########
src/bthread/timer_thread.cpp:
##########
@@ -365,10 +365,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 doesn’t convey what is
being measured or its unit. Renaming to something specific like `task_latency`
/ `task_latency_us` (or `task_run_latency`) would make dashboards and alerts
clearer and reduce the chance of confusion/collision with other `task` metrics.
--
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]
