ARM64 threading optimization - atomic ordering and cache line alignment - #95
Conversation
|
Welcome @mohakgoel1! It looks like this is your first PR to tikv/yatp 🎉 |
|
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)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWalkthroughThis change replaces selected ChangesAtomic synchronization updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The optimization may not fully prevent cache-line contention on some AArch64 systems, limiting the expected performance improvement. The PR remains mergeable with explicit owner follow-up to verify or provide target-specific alignment. Suggested reviewers: 🚥 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/pool/spawn.rs`:
- Around line 17-22: Update the CACHE_LINE_SIZE architecture configuration in
spawn.rs to separate aarch64 from the 128-byte architectures and set it to 256
bytes. Apply the corresponding 256-byte #[repr(C, align(...))] alignment and
padding to the active_workers-related structures or fields in the referenced
sections, while preserving the existing 128-byte behavior for x86_64 and
powerpc64.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4417f7c0-9b58-4156-8c5d-381eefa116bd
📒 Files selected for processing (4)
src/pool/builder.rssrc/pool/spawn.rssrc/queue/multilevel.rssrc/task/future.rs
Signed-off-by: mohakgoel1 <mohak.goel@fujitsu.com>
Signed-off-by: mohakgoel1 <mohak.goel@fujitsu.com>
9210249 to
cffc852
Compare
| QueueCore { | ||
| global_queue, | ||
| active_workers: AtomicUsize::new(config.max_thread_count << WORKER_COUNT_SHIFT), | ||
| active_workers: CacheAligned::new(config.max_thread_count << WORKER_COUNT_SHIFT), |
There was a problem hiding this comment.
How about crossbeam::utils::CachePadded?
There was a problem hiding this comment.
Hi @BusyJay,
Thanks for the review and for pointing this out.
We considered crossbeam_utils::CachePadded but moved away from it for two reasons that came up earlier in this review:
- On 32-bit arm, CachePadded aligns to only 32 bytes, which may not fully prevent false sharing on all arm32 cache line sizes.
- On aarch64, CachePadded aligns to 128 bytes, but the Fujitsu A64FX a production aarch64 server chip used in supercomputing uses 256-byte cache lines. With 128-byte alignment, active_workers could still share a cache line with an adjacent QueueCore field on that platform.
The explicit #[repr(C, align(N))] struct with per-platform #[cfg] blocks gives us full control over both size and alignment on every target, covering all the above cases correctly. The comment above the struct documents this reasoning so future readers understand why CachePadded was not used.
There was a problem hiding this comment.
Can you also send a patch to crossbeam and see whether they accept the changes?
There was a problem hiding this comment.
If they don't, we can reuse definitions from crossbeam by reexporting them in wrapper mode as a fallback, and only define what we know to be wrong in our own crate.
There was a problem hiding this comment.
Thank you for the suggestion. Our thought is to go with Option 2 the wrapper/fallback pattern feels like the right approach here. It keeps the custom logic minimal by only overriding the two platforms where crossbeam's alignment is known to be insufficient (aarch64 at 256 bytes and arm at 64 bytes), and re-exports CachePadded directly for everything else. This avoids duplicating crossbeam's full platform logic in yatp, is easier to maintain long term, and if crossbeam ever updates their alignment values upstream, removing our overrides becomes a one-line cleanup.
What are your thoughts on proceeding with Option 2?
There was a problem hiding this comment.
Hi @BusyJay, Could you please confirm if we should proceed with the wrapper/fallback approach?
There was a problem hiding this comment.
Hi @BusyJay, Just following up on my previous comment. Could you please confirm if we can proceed with the wrapper/fallback approach? Thanks!
There was a problem hiding this comment.
Hi @BusyJay,
We've pushed a commit removing the custom CacheAligned struct and replacing it with CachePadded for active_workers in QueueCore. On aarch64 this gives 128-byte alignment, which is safe and correct.
Let me know if this looks good to you, or if you'd like any further changes before merging.
…ive_workers Signed-off-by: mohakgoel1 <mohak.goel@fujitsu.com>
|
Hi @BusyJay, @Connor1996, @gengliqi, We have addressed the previous feedback and updated the PR accordingly. Could you please take a look when you get a chance? Your review and feedback would be greatly appreciated. |
|
Thank you @mohakgoel1 for your contribution~ |
Thank you for taking the time to review the PR and for your valuable feedback. I really appreciate your support and help in getting it merged! |
4 participants
yatp: ARM64 threading optimization, Atomic ordering and cache line alignment
Background
yatp was originally optimized primarily for x86. On ARM64, unnecessary SeqCst atomic ordering and false sharing can add performance overhead due to ARM's weaker memory model and 128-byte cache lines on server CPUs such as AWS Graviton and ARM Neoverse.
Changes
1. Atomic ordering
Replaced unnecessary SeqCst operations with the minimum required memory ordering across 33 operations in 4 files:
Used Acquire, Release, or AcqRel based on the synchronization requirements of each operation.
These changes reduce unnecessary memory barriers on ARM64 while preserving the existing synchronization behavior. No logic changes were made.
2. Cache line alignment
Added a custom CacheAligned struct in pool/spawn.rs to prevent false sharing between active_workers and global_queue.
The struct uses explicit alignment:
This ensures appropriate cache line separation across supported architectures.
Benchmark Results
TPC-C benchmark with 1 TiKV server and 750 warehouses:
The optimization provides a larger improvement on ARM64, while also showing a smaller positive impact on x86.
Correctness
This PR is a joint contribution by:
Summary by CodeRabbit
Performance
Bug Fixes