Copilot commented on code in PR #3247:
URL: https://github.com/apache/brpc/pull/3247#discussion_r2978673789
##########
src/butil/time.h:
##########
@@ -301,11 +301,6 @@ inline int64_t cpuwide_time_ns() {
} else if (!cpu_freq) {
// Lack of necessary features, return system-wide monotonic time
instead.
return monotonic_time_ns();
Review Comment:
`cpuwide_time_ns()` can fall through without returning a value when
`detail::invariant_cpu_freq` is negative (e.g., still `-1` before
`init_invariant_cpu_freq()` runs, or if initialization fails and leaves it <
0). This is undefined behavior. Please ensure the function always returns a
value for the `< 0` case (e.g., treat it like the `0` case and return
`monotonic_time_ns()`, or perform a safe one-time initialization here).
```suggestion
return monotonic_time_ns();
} else {
// Negative frequency (e.g. uninitialized or failed initialization),
// fall back to system-wide monotonic time to avoid undefined
behavior.
return monotonic_time_ns();
```
##########
src/butil/time.cpp:
##########
@@ -153,6 +153,18 @@ int64_t read_invariant_cpu_frequency() {
}
int64_t invariant_cpu_freq = -1;
+
+static void __attribute__((constructor)) init_invariant_cpu_freq() {
+ int64_t baseFreq = -1;
+#if defined(__aarch64__)
+ __asm__ __volatile__("mrs %0, CNTFRQ_EL0" : "=r"(baseFreq));
+#else
+ baseFreq = detail::read_invariant_cpu_frequency();
+#endif
+
+ detail::invariant_cpu_freq = baseFreq;
+}
+
Review Comment:
Using `__attribute__((constructor))` in a C++ TU is compiler-specific and
can be problematic for portability/toolchains. This repo already has
`BAIDU_GLOBAL_INIT()` (which avoids the attribute in C++) and many places use
`pthread_once`/function-local statics for one-time init; consider switching to
an established mechanism so initialization is reliable across supported
compilers.
```suggestion
static int64_t init_invariant_cpu_freq() {
int64_t baseFreq = -1;
#if defined(__aarch64__)
__asm__ __volatile__("mrs %0, CNTFRQ_EL0" : "=r"(baseFreq));
#else
baseFreq = detail::read_invariant_cpu_frequency();
#endif
return baseFreq;
}
int64_t invariant_cpu_freq = init_invariant_cpu_freq();
```
##########
src/butil/time.cpp:
##########
@@ -153,6 +153,18 @@ int64_t read_invariant_cpu_frequency() {
}
int64_t invariant_cpu_freq = -1;
+
+static void __attribute__((constructor)) init_invariant_cpu_freq() {
+ int64_t baseFreq = -1;
+#if defined(__aarch64__)
+ __asm__ __volatile__("mrs %0, CNTFRQ_EL0" : "=r"(baseFreq));
+#else
+ baseFreq = detail::read_invariant_cpu_frequency();
+#endif
+
+ detail::invariant_cpu_freq = baseFreq;
+}
Review Comment:
`init_invariant_cpu_freq()` runs unconditionally at load time and, on
non-aarch64, reads `/proc/cpuinfo` even in builds where `cpuwide_time_ns()`
uses `clock_gettime` and never consults `detail::invariant_cpu_freq` (i.e.,
`!BAIDU_INTERNAL && !__aarch64__`). This can add avoidable startup I/O/latency.
Consider gating initialization behind the same preprocessor conditions as the
fast-path usage, or lazily initializing on first use.
##########
src/butil/time.cpp:
##########
@@ -153,6 +153,18 @@ int64_t read_invariant_cpu_frequency() {
}
int64_t invariant_cpu_freq = -1;
+
+static void __attribute__((constructor)) init_invariant_cpu_freq() {
+ int64_t baseFreq = -1;
+#if defined(__aarch64__)
+ __asm__ __volatile__("mrs %0, CNTFRQ_EL0" : "=r"(baseFreq));
+#else
+ baseFreq = detail::read_invariant_cpu_frequency();
+#endif
+
+ detail::invariant_cpu_freq = baseFreq;
Review Comment:
Local variable name `baseFreq` doesn’t match the surrounding naming style in
this file (which predominantly uses snake_case like `invariant_tsc`,
`seen_decpoint`, `endp`). Renaming to something like `base_freq` (and ideally
indicating units, e.g. `_hz`) would improve consistency and readability.
```suggestion
int64_t base_freq_hz = -1;
#if defined(__aarch64__)
__asm__ __volatile__("mrs %0, CNTFRQ_EL0" : "=r"(base_freq_hz));
#else
base_freq_hz = detail::read_invariant_cpu_frequency();
#endif
detail::invariant_cpu_freq = base_freq_hz;
```
--
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]
