fix: use calibrated TSC frequency for work-loop budgets - #495
fix: use calibrated TSC frequency for work-loop budgets#495thweetkomputer wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds a private Abseil base link and updates x86-64 TSC frequency initialization to use Abseil’s nominal CPU frequency, convert it to cycles per microsecond, clamp it, and store it. ChangesTSC frequency initialization
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/storage/shard.cpp`:
- Around line 1439-1444: Update the initialization around
tsc_cycles_per_microsecond_ so it does not use
absl::base_internal::NominalCPUFrequency() when the reported frequency is
unavailable or implausibly low. Use a verified calibrated cycles-per-microsecond
value, or make ReadTimeMicroseconds() fall back to monotonic time, while
preserving the existing release store for valid frequency measurements.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17f0665b-3c86-447e-958c-a2bf741fef33
📒 Files selected for processing (2)
CMakeLists.txtsrc/storage/shard.cpp
| const double frequency_hz = | ||
| absl::base_internal::NominalCPUFrequency(); | ||
| const uint64_t cycles_per_microsecond = std::max<uint64_t>( | ||
| 1, static_cast<uint64_t>(frequency_hz / 1'000'000.0)); | ||
| tsc_cycles_per_microsecond_.store(cycles_per_microsecond, | ||
| std::memory_order_release); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the repository-selected Abseil implementation and its fallback paths.
fd -HI '^sysinfo\.(cc|h)$' . | while IFS= read -r file; do
rg -n -C 12 'NominalCPUFrequency|GetNominalCPUFrequency|return 1\.0|tsc_freq_khz' "$file"
done
# Identify the selected Abseil version or source pin.
rg -n -C 3 'absl|ABSL|FetchContent|GIT_TAG' -g 'CMakeLists.txt' -g '*.cmake' .Repository: eloqdata/eloqstore
Length of output: 19988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate Abseil declarations/versions in cmake and deps=="
rg -n -C 8 'abseil|ABSL|absl|FetchContent_Declare\(|absl-base|absl_core|base_internal|sysinfo' cmake CMakeLists.txt rust/eloqstore-sys/vendor/CMakeLists.txt || true
echo
echo "== local sysinfo files =="
fd -HI 'sysinfo\.(cc|h)$' . -t f | sort
echo
echo "== repository-wide selected abseil sources =="
git ls-files | rg 'abseil|absl.*sysinfo|absl/base/internal/sysinfo' || true
echo
echo "== src/storage/shard.cpp header and TSC-related context =="
wc -l src/storage/shard.cpp
sed -n '1,40p;1360,1470p' src/storage/shard.cpp
echo
echo "== all NominalCPUFrequency references =="
rg -n -C 5 'NominalCPUFrequency|absl::base_internal' .Repository: eloqdata/eloqstore
Length of output: 29588
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== external/abseil candidate version/source markers =="
for f in external/abseil/CMakeLists.txt external/abseil/git_rev external/abseil/source external/abseil/version external/abseil/src/version external/abseil/README.md external/abseil/src/absl/base/internal/sysinfo.cc external/abseil/absl/base/internal/sysinfo.cc; do
if [ -e "$f" ]; then
echo "--- $f"
sed -n '1,120p' "$f"
fi
done
echo
echo "== external/abseil sysinfo implementation =="
if [ -f external/abseil/absl/base/internal/sysinfo.cc ]; then
FILE=external/abseil/absl/base/internal/sysinfo.cc
else
FILE=external/abseil/src/absl/base/internal/sysinfo.cc
fi
wc -l "$FILE"
rg -n -C 20 'GetNominalCPUFrequency|return_freq|NominalCPUFrequency|return 1\.0|1\.0|tsc_freq_khz|cpufreq|ReadFile|sysinfo' "$FILE"
echo
echo "== local TSC consumer context =="
wc -l src/storage/shard.cpp
sed -n '1470,1520p' src/storage/shard.cpp
rg -n -C 4 'ReadTimeMicroseconds|InitializeTscFrequency|capped_spin|sleep_for|work-loop|budget|deadline' src/storage src -g '*.cpp' -g '*.h' | head -n 240Repository: eloqdata/eloqstore
Length of output: 338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tracked Abseil/source paths =="
git ls-files | rg '(^|/)abseil/|absl/base/internal/sysinfo\.(cc|h)$|CMakeLists\.txt$' | sed -n '1,220p'
echo
echo "== local filesystem directories/subtrees =="
python3 - <<'PY'
import os
for root in ['external', 'src', 'rust/eloqstore-sys', '.']:
if not os.path.isdir(root):
continue
print(f"-- {root}")
for entry in sorted(os.listdir(root)[:200]):
path=os.path.join(root, entry)
print(entry, 'DIR' if os.path.isdir(path) else os.path.getsize(path) if os.path.isfile(path) else '?')
PY
echo
echo "== candidate sysinfo paths under tracked files =="
git ls-files 'external/**' 'src/**' 'rust/**' 'cmake/**' 'CMakeLists.txt' \
| rg 'sysinfo|abseil|absl|CMakeLists.txt' \
| sort \
| rg 'sysinfo' || true
echo
echo "== explicit Abseil versions if present =="
rg -n -C 4 'GIT_TAG|GIT_REPOSITORY|GIT_SUBMODULE|abseil|ABSL' .gitmodules CMakeLists.txt cmake/external cmake dependencies.cmake external/** rust/eloqstore-sys/** -E 'abseil|absl|GIT_TAG|absl' 2>/dev/null | sed -n '1,260p' || trueRepository: eloqdata/eloqstore
Length of output: 1614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local external/abseil directory listing (top levels) =="
find external/abseil -maxdepth 2 -printf '%p %Y\n' 2>/dev/null | sort | sed -n '1,220p'
echo
echo "== local sysinfo source candidates =="
while IFS= read -r file; do
echo "--- $file"
wc -l "$file"
rg -n -C 20 'GetNominalCPUFrequency|NominalCPUFrequency|return_freq|return 1\.0|1\.0|tsc_freq_khz|cpufreq|ReadFile|sysinfo' "$file" || true
done < <(find external/abseil -type f -name 'sysinfo.cc' -print 2>/dev/null | sort)
echo
echo "== version markers and submodule hints =="
if [ -f .gitmodules ]; then
cat .gitmodules
fi
for file in VERSION README.md cmake/dependencies.cmake CMakeLists.txt; do
if [ -f "$file" ]; then
echo "--- $file"
rg -n -C 4 'abseil|GIT_TAG|GIT_REPOSITORY|version|external/abseil' "$file" || true
fi
doneRepository: eloqdata/eloqstore
Length of output: 325
🌐 Web query:
Abseil NominalCPUFrequency return 1.0 source absl/base/internal/sysinfo GetNominalCPUFrequency fallback
💡 Result:
In the Abseil C++ library, the function absl::base_internal::GetNominalCPUFrequency (and the public-facing NominalCPUFrequency which calls it) returns 1.0 as a fallback value when it cannot successfully determine the CPU frequency through platform-specific APIs [1][2][3]. The value 1.0 serves as a safe, non-zero default to prevent issues like division-by-zero that could occur if the library attempted to use a 0.0 frequency in performance calculations [2][3]. The implementation of GetNominalCPUFrequency uses a series of platform-dependent checks to detect the CPU frequency [1][4][3]. If these checks fail—such as when a system call returns an error, registry keys are inaccessible (e.g., in UWP applications), or a file cannot be read from /sys/devices/system/cpu—the function executes a fallback return statement of 1.0 [1][2][5][3]. Common scenarios where this fallback is triggered include: - UWP (Universal Windows Platform) applications, where access to the Windows registry is restricted [5][3]. - Systems where the expected platform-specific APIs (such as sysctl on certain BSD or macOS configurations) fail to provide the frequency data [6]. - Environments where the necessary system files (e.g., /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq) are missing or inaccessible [1][7]. Because NominalCPUFrequency is designed to be called before main and before memory allocation is fully initialized, it is implemented to be lightweight and must not allocate memory, which is why it relies on these direct, low-level platform inquiries rather than complex configuration lookups [1][8][3].
Citations:
- 1: https://github.com/abseil/abseil-cpp/blob/master/absl/base/internal/sysinfo.cc
- 2: https://docs.ros.org/en/api/grpc/html/bloaty_2third__party_2abseil-cpp_2absl_2base_2internal_2sysinfo_8cc_source.html
- 3: https://android.googlesource.com/platform/external/abseil-cpp/+/0b8558168457876b42e68f851bda7da6b2f46157/absl/base/internal/sysinfo.cc
- 4: https://chromium.googlesource.com/external/github.com/abseil/abseil-cpp/+/fcb104594b0bb4b8ac306cb2f55ecdad40974683/absl/base/internal/sysinfo.cc
- 5: UWP doesn't allow reading regkeys abseil/abseil-cpp#594
- 6: NominalCPUFrequency Test from SysInfoTest Suite Fails on M1 Mac abseil/abseil-cpp#1053
- 7: https://github.com/abseil/abseil-cpp/blob/889ddc99/absl/base/internal/sysinfo.cc
- 8: https://github.com/abseil/abseil-cpp/blob/b6c18169/absl/base/internal/sysinfo.cc
Reject the NominalCPUFrequency() fallback.
Nil/unavailable platform frequency lookup makes absl::base_internal::NominalCPUFrequency() return 1.0. The std::max<uint64_t>(1, ...) clamp then makes ReadTimeMicroseconds() advance 1 million times faster than real time. This causes work-loop budgets to run too long and delayed-request deadlines to expire early. Use a verified calibrated fallback or fall back to monotonic time when the reported frequency is not plausible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/shard.cpp` around lines 1439 - 1444, Update the initialization
around tsc_cycles_per_microsecond_ so it does not use
absl::base_internal::NominalCPUFrequency() when the reported frequency is
unavailable or implausibly low. Use a verified calibrated cycles-per-microsecond
value, or make ReadTimeMicroseconds() fall back to monotonic time, while
preserving the existing release store for valid frequency measurements.
1 participant
Summary
sleep_for()-based TSC calibration with Abseil's calibrated raw TSC frequencyabsl::baseexplicitly becauseNominalCPUFrequency()is used directlycntfrq_el0path unchangedRoot cause
The previous x86 calibration divided elapsed TSC ticks by the requested sleep duration.
sleep_for(1000us)is allowed to return later than requested when the initializing thread is descheduled. Using 1000 rather than the actual monotonic elapsed time therefore overestimates the TSC frequency. Repeated stable samples do not eliminate this systematic error when the scheduler delay is similar between samples.An overestimated frequency makes
ReadTimeMicroseconds()advance too slowly, so the 20us cooperative-yield budget, the per-round work-loop budget, and delayed-request deadlines can all run late.Abseil's calibration pairs raw TSC samples with actual monotonic-clock readings and retries with increasing measurement intervals. Its
NominalCPUFrequency()is the frequency corresponding to the rawrdtscvalue used here.Impact
Work-loop and cooperative-yield time budgets use a frequency calibrated against actual elapsed time and no longer depend on scheduler oversleep during EloqStore initialization.
Validation
Local tests were intentionally not run; the repository's amd64 and arm64 GitHub CI jobs are expected to validate this change.
Summary by CodeRabbit