Skip to content

cputime optimize for arm64 #3247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dwh110 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cputime optimize for arm64 #3247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/butil/time.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +158 to +165
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;
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;

Copilot uses AI. Check for mistakes.
}
Comment on lines +161 to +166
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

Comment on lines 155 to +167
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;
}
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();

Copilot uses AI. Check for mistakes.
} // namespace detail

} // namespace butil
7 changes: 1 addition & 6 deletions src/butil/time.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ extern int64_t invariant_cpu_freq;
// note: Inlining shortens time cost per-call for 15ns in a loop of many
// calls to this function.
inline int64_t cpuwide_time_ns() {
#if !defined(BAIDU_INTERNAL)
#if !defined(BAIDU_INTERNAL) && !defined(__aarch64__)
// nearly impossible to get the correct invariant cpu frequency on
// different CPU and machines. CPU-ID rarely works and frequencies
// in "model name" and "cpu Mhz" are both unreliable.
Expand All @@ -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();
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
return monotonic_time_ns();
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();

Copilot uses AI. Check for mistakes.
} else {
// Use a thread-unsafe method(OK to us) to initialize the freq
// to save a "if" test comparing to using a local static variable
detail::invariant_cpu_freq = detail::read_invariant_cpu_frequency();
return cpuwide_time_ns();
}
#endif // defined(BAIDU_INTERNAL)
}
Expand Down
Loading