feat(atomic64): add 64-byte CachePadded alignment to eliminate CPU false sharing - #568
feat(atomic64): add 64-byte CachePadded alignment to eliminate CPU false sharing#568Aditya-9-6 wants to merge 1 commit into
Conversation
|
Welcome @Aditya-9-6! It looks like this is your first PR to tikv/rust-prometheus 🎉 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults 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)
📝 WalkthroughWalkthroughAdds a public 64-byte-aligned ChangesCache-padded wrapper
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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/atomic64.rs`:
- Around line 249-280: Implement the Atomic trait for CachePadded<P> when P:
Atomic, forwarding new, get, set, inc_by, and dec_by to self.inner while
preserving the associated type. Add coverage using CachePadded<AtomicU64> as the
GenericCounter value type to verify the trait implementation supports generic
counter operations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| pub struct CachePadded<P> { | ||
| inner: P, | ||
| } | ||
|
|
||
| impl<P> CachePadded<P> { | ||
| /// Create a new 64-byte cache-padded atomic metric value. | ||
| pub const fn new(inner: P) -> Self { | ||
| Self { inner } | ||
| } | ||
|
|
||
| /// Consumes the wrapper and returns the inner value. | ||
| pub fn into_inner(self) -> P { | ||
| self.inner | ||
| } | ||
| } | ||
|
|
||
| impl<P> Deref for CachePadded<P> { | ||
| type Target = P; | ||
|
|
||
| #[inline] | ||
| fn deref(&self) -> &P { | ||
| &self.inner | ||
| } | ||
| } | ||
|
|
||
| impl<P> DerefMut for CachePadded<P> { | ||
| #[inline] | ||
| fn deref_mut(&mut self) -> &mut P { | ||
| &mut self.inner | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'GenericCounter|CachePadded|impl.*Atomic.*for' src --glob '*.rs' || trueRepository: tikv/rust-prometheus
Length of output: 16790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Atomic trait and CachePadded tests/type methods.
sed -n '1,110p' src/atomic64.rs
printf '\n--- CachePadded and tests ---\n'
sed -n '247,335p' src/atomic64.rs
printf '\n--- GenericCounter construction methods ---\n'
sed -n '19,70p' src/counter.rs
printf '\n--- Read-only compile-time probe: does CachePadded satisfy Atomic bound? ---\n'
cat > /tmp/probe.rs <<'RS'
trait Atomic {
type T;
fn new(t: Self::T) -> Self;
}
struct Padded<P>(P);
impl<P> Padded<P> {
const fn new(inner: P) -> Self { Self(inner) }
}
struct Counter<P: Atomic>(P);
fn main() {
let wrapped = Padded::new(42u64);
Counter(wrapped);
}
RS
if command -v rustc >/dev/null 2>&1; then
rustc /tmp/probe.rs 2>&1 | sed 's#/tmp/probe.rs#<probe>`#g`' || true
else
echo "rustc not available"
fiRepository: tikv/rust-prometheus
Length of output: 6348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Value methods that use Atomic bounds ---'
rg -n -C 4 'impl<.*Atomic|struct Value<|fn inc_by|fn dec_by|fn set\(|fn get\(' src/value.rsRepository: tikv/rust-prometheus
Length of output: 1262
Implement Atomic for CachePadded<P>.
GenericCounter<P> requires P: Atomic, and Value<P> calls P::new, P::T::from_i64, get, set, inc_by, and dec_by on the inner value. CachePadded<P> only provides new and into_inner, so it cannot be used as a generic counter value type; the existing get/inc_by test only works via deref method lookup. Add impl<P: Atomic> Atomic for CachePadded<P> forwarding those methods to self.inner, and cover CachePadded<AtomicU64> with GenericCounter.
🤖 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/atomic64.rs` around lines 249 - 280, Implement the Atomic trait for
CachePadded<P> when P: Atomic, forwarding new, get, set, inc_by, and dec_by to
self.inner while preserving the associated type. Add coverage using
CachePadded<AtomicU64> as the GenericCounter value type to verify the trait
implementation supports generic counter operations.
…lse sharing Signed-off-by: Aditya-9-6 <aditya-9-6@users.noreply.github.com>
ea48086 to
4399e66
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Summary
Under high multi-threaded concurrency (e.g. 100k+ RPS proxy workloads), storing atomic counters (
AtomicU64,AtomicF64,AtomicI64) in contiguous memory vectors (CounterVec,GaugeVec) causes adjacent atomic metrics to share the same 64-byte L1 CPU cache line. When multiple worker threads update adjacent counters simultaneously, CPU cores experience hardware cache-line ping-ponging (False Sharing).This PR introduces
CachePadded<P>, a 64-byte#[repr(align(64))]wrapper that aligns atomic metric counters to hardware cache-line boundaries, eliminating cross-thread cache contention.Proposed Changes
CachePadded<P>: Added a 64-byte aligned wrapper struct insrc/atomic64.rswithDerefandDerefMutimplementations.test_cache_padded()insrc/atomic64.rsverifying 64-byte memory alignment (std::mem::align_of_val) and deref functionality.Verification
cargo test --lib: Allatomic64unit tests passed cleanly.cargo clippy --lib: Passed with 0 warnings/errors.Summary by CodeRabbit
New Features
CachePaddedwrapper that aligns values to 64-byte cache lines.Tests