perf: Optimize is_owner rate limit checks with zero allocation tracking#971
perf: Optimize is_owner rate limit checks with zero allocation tracking#971
Conversation
Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsistentHashRing::is_owner was rewritten to compute ownership by hashing and scanning the ring with explicit termination and fixed-size stack tracking; the consistent_hash module was made public (doc-hidden); a Criterion benchmark for is_owner was added to model_gateway. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request optimizes the is_owner method in the ConsistentHashRing to be zero-allocation and introduces a benchmark suite to track its performance. Additionally, the consistent_hash module is now publicly accessible. Feedback was provided to simplify the is_owner logic by using Iterator::chain for a more idiomatic approach to handling ring wrap-around, which would enhance readability and maintainability without impacting performance.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17b7cda58b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
|
Hi @ppraneth, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
Description
Problem
The
is_ownermethod in the consistent hash ring was previously delegating toget_owners, which heavily allocated memory just to return a boolean check. To verify ownership, the original code had to allocate a newVec, allocate aHashSet, clone strings repeatedly, and perform an extra string allocation for every check. This created unnecessary memory and CPU overhead during high throughput scenarios.Solution
This PR eliminates all temporary vectors, dynamic HashSets, and string clones from the
is_ownerexecution path. It introduces a fast, short circuiting iterator that tracks seen unique nodes using a small stack allocated array. Because the number of replica owners is very small, a linear scan over a fixed array is noticeably faster than hashing, allowing us to hit true zero memory allocations.Changes
is_ownerfrom the allocatingget_ownerspipeline in the mesh crate.HashSetwith a tiny stack bounded array.consistent_hashmodule public to allow for direct isolated benchmarking.model_gatewayto measure execution times over thousands of keys.Test Plan
A new benchmark
consistent_hash_benchwas added to track cache hits and misses against the ring. You can reproduce the metrics by running:Before changes:
Average execution time was approximately 173.00 µs.
After changes:
Average execution time dropped to approximately 29.90 µs.
This is an 82.7 percent performance improvement per block. also verified that all tests continue to pass and there are no remaining clippy warnings.
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Performance
API Changes
Tests / Benchmarks