Wave-33 C00: optional jemalloc feature (soft) - #277
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Gate tikv-jemallocator behind --features jemalloc (Unix-only) so default and Windows builds keep the system allocator; add SelfCheck docs and soft ops-load CI. Co-authored-by: Cursor <cursoragent@cursor.com>
abd4a92 to
c75fe39
Compare
| otel-metrics = [] | ||
| # Soft optional jemalloc global allocator (Unix). Default builds stay on the | ||
| # system allocator — Windows-safe / unchanged. Enable: --features jemalloc | ||
| jemalloc = ["dep:tikv-jemallocator"] |
There was a problem hiding this comment.
SUGGESTION: jemalloc feature references a cfg(unix)-only optional dependency via dep:
jemalloc = ["dep:tikv-jemallocator"] points at a dependency declared solely under [target.'cfg(unix)'.dependencies]. On non-Unix targets the dependency does not exist, so this dep: reference is inert there. Modern Cargo resolves this fine (you confirmed cargo check --features jemalloc works on Windows), but please verify the repo's standard cargo test --all-features loop on Windows still resolves — --all-features also enables jemalloc, and some Cargo versions error with "feature includes dep:, but is not a dependency for target ..." for target-specific optional deps. If it ever breaks, gate the feature behind a cfg(unix) resolver or make the optional dep unconditional (non-target) optional.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| [void](Write-Check -Label "jemalloc not in default features" -Ok $true) | ||
|
|
||
| if ($mainRs -notmatch 'cfg\(all\(feature = "jemalloc", unix\)\)') { |
There was a problem hiding this comment.
SUGGESTION: cfg gate check is argument-order sensitive
The regex cfg\(all\(feature = "jemalloc", unix\)\) only matches that exact argument order. A functionally equivalent reorder such as cfg(all(unix, feature = "jemalloc")) (or a future refactor that wraps the gate) would fail the SelfCheck and break the soft test even though the allocator is still correctly Unix+feature gated. Consider asserting both conditions independently — e.g. match feature = "jemalloc" and unix inside a cfg(all(...)) — so reordering does not regress the check.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found (all non-blocking) | Recommendation: Merge with optional follow-ups Overview
Issue Details (click to expand)SUGGESTION
Consider (non-line)
Files Reviewed (11 files)
Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 93.5K · Output: 18.5K · Cached: 227K |
Summary
jemallocCargo feature forsl-daemonusingtikv-jemallocator(Unix-only#[global_allocator]); default/Windows builds unchanged on the system allocatordocs/ops/jemalloc.mdwith hermetic SelfCheck (scripts/jemalloc-check.ps1+tests/jemalloc_soft.rs)jemallocjob inops-load.yml(continue-on-error) builds--features jemallocon UbuntuTest plan
pwsh ./scripts/jemalloc-check.ps1 -SelfCheckcargo test --test jemalloc_soft --lockedcargo build --locked(default, Windows)cargo check --features jemalloc --locked(Windows no-op for allocator; feature resolves)ops-loadjemalloc job on Ubuntu