perf(compaction/gc): cut worker-thread occupation that stalls foreground serving

[liunyl]

Problem

EloqStore compaction/GC coroutines run on the same shard worker thread as foreground request serving — and in module mode that worker also drives the TxProcessor on the same core. The scheduler (WorkOneRound/ExecuteReadyTasks) cannot return to serve foreground requests until the running compaction/GC coroutine yields, so a single long non-yielding compaction/GC segment stalls foreground SET/GET for its whole duration. This is the direct cause of deep-tail latency spikes during compaction (the foreground path does no disk I/O of its own, yet both reads and writes spike symmetrically while compaction runs).

Investigation localized the long segments to the file-GC path: per-resume occupation of tens of ms in AugmentRetainedFilesFromBranchManifests (manifest replay + full mapping-table walk) and in DeleteUnreferencedLocal{,Segment}Files (CloseFiles/DeleteFiles building one io_uring SQE per file in a single uninterrupted loop when a partition has accumulated many dead files).

Changes

• Skip the active branch's redundant manifest replay in GC. In AugmentRetainedFilesFromBranchManifests, the active branch's retained files are already derived from the in-memory mapping by BuildRetainedFiles, and the in-memory RootMeta is at least as current as the on-disk manifest (same invariant SeedActiveBranchGuardFromInMemory relies on). So re-reading + replaying the active branch's current-term manifest from disk was pure redundant work — it was the most frequent long GC segment. Skipped only when the in-memory RootMeta is present; non-active branches / archives still replay from disk (they may be updated by other instances and have no trustworthy in-memory mapping here).
• Chunk IouringMgr::DeleteFiles and CloseFiles (128 ops + WaitIo per chunk) so a GC pass over many accumulated dead files no longer builds all unlink/close SQEs in one uninterrupted stretch; WaitIo between chunks lets the worker serve foreground.
• Cooperative time-budgeted yield (MaybeYieldForCompaction / eloqstore_compaction_yield_budget_us, default 500µs) called from the compaction and file-GC loops.

Compaction concurrency is intentionally not given a separate cap: BackgroundWrite (compaction/GC) is a write task counted in num_active_write_, so the existing eloq_store_max_write_concurrency already bounds it together with foreground batch writes.

Verification

• Reproduced compaction-time worker occupation under a write-heavy (overwrite) workload; the AugmentRetainedFiles long segments (previously the most frequent, ~20ms) are eliminated.
• Correctness: data fully intact across restart-recovery after heavy compaction (GC does not over-delete files referenced by the durable manifest); no GC errors.

Note / follow-up

The residual long GC segments are now CloseFiles/DeleteFiles over a partition's accumulated dead files; the lever there is reducing per-GC file count (more incremental compaction/GC). Also, for the non-active-branch path, Replayer::Replay (manifest reconstruction) currently has no yield — a candidate for a follow-up when multi-branch/archive manifests are present.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added a configurable cooperative yield time budget (eloqstore_yield_budget_us, default 20) based on elapsed time since the task was last resumed.
• Performance Improvements
  • Updated background compaction and file-scanning loops to cooperatively yield using time-budgeted MaybeYield() polling for better responsiveness.
  • Improved cleanup behavior by chunking large file close/unlink operations to avoid long scheduler stalls.
  • Reduced redundant active-branch manifest replay work during file GC when safe.
• Documentation
  • Clarified that fdatasync is intentionally preserved during cleanup to prevent major performance regressions.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ liunyl
❌ Ubuntu

---

Ubuntu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• ✅ Review completed - (🔄 Check again to review again)

Walkthrough

Adds cooperative time-budgeted yielding for compaction and file-GC coroutine segments using a configurable eloqstore_yield_budget_us flag (default 20 µs). Shard gains CurResumeElapsedUs() method and resume timestamp tracking; MaybeYield() is inserted across compaction, file-GC, and async I/O loops to yield when elapsed time exceeds the budget. Close/unlink operations are converted to bounded chunks of 128. Manifest replay is optimized away when in-memory RootMeta is already populated for the active branch.

Changes

Cooperative Time-Budgeted Yielding for Compaction/File-GC

Layer / File(s)	Summary
Shard timing measurement contract include/storage/shard.h, include/tasks/task.h	Shard gains cur_resume_start_us_ field to store resume timestamp, CurResumeElapsedUs() method to query elapsed microseconds, and StartTask resets the timestamp. MaybeYield() is declared in the eloqstore namespace with time-budget documentation.
Time-budget flag and MaybeYield implementation src/storage/shard.cpp, src/tasks/task.cpp	eloqstore_yield_budget_us gflag defined (default 20 µs). MaybeYield() implemented with null-safety; calls YieldToLowPQ() when elapsed time exceeds the configured budget.
Resume-timestamp instrumentation in shard scheduler src/storage/shard.cpp	ExecuteReadyTasks() stamps cur_resume_start_us_ before resuming tasks in both normal and low-priority loops. Compaction task comment updated to document write-task nature and concurrency bounding via GetBackgroundWrite().
Time-budgeted yields in compaction loops src/tasks/background_write.cpp	MaybeYield() inserted in DoCompactDataFile and DoCompactSegmentFile per-file loops to bound per-resume CPU time. Old count-based yield state removed; comments replace coarse yielding with time-budgeted behavior.
Time-budgeted yields in file-GC scanning src/file_gc.cpp	Adds kYieldPollStride constant for amortized polling in cheap scan loops. MaybeYield() inserted in ListLocalFiles, ClassifyFiles, DeleteUnreferencedLocalFiles, DeleteUnreferencedLocalSegmentFiles, and DeleteUnreferencedCloudSegmentFiles to prevent long uninterrupted scanning on large file sets.
Manifest replay optimization and fdatasync documentation src/file_gc.cpp	AugmentRetainedFilesFromBranchManifests skips disk manifest replay for the active branch when in-memory RootMeta is already populated for that branch/term. MaybeYield() added per manifest. Block comments document intentional fdatasync-before-unlink ordering and its measured ~10x writeback stall impact.
Chunked async I/O operations src/async_io_manager.cpp	CloseFiles and DeleteFiles now submit SQEs and wait in bounded chunks of 128 instead of single large batches, enabling per-chunk yields and preventing worker-thread stalls from massive close/unlink operations.

Sequence Diagram(s)

sequenceDiagram
  participant BgLoop as Background Loop<br/>(compaction/file-GC)
  participant MaybeYield as MaybeYield()
  participant Shard
  participant Scheduler as ExecuteReadyTasks()

  Scheduler->>Shard: cur_resume_start_us_ = ReadTimeMicroseconds()
  Scheduler->>BgLoop: resume coroutine
  loop per file / entry
    BgLoop->>MaybeYield: MaybeYield()
    MaybeYield->>Shard: CurResumeElapsedUs()
    Shard-->>MaybeYield: elapsed_us
    alt elapsed_us >= eloqstore_yield_budget_us (20 µs)
      MaybeYield->>BgLoop: YieldToLowPQ()
      BgLoop-->>Scheduler: suspend to low-priority queue
      Scheduler->>Shard: cur_resume_start_us_ = ReadTimeMicroseconds()
      Scheduler->>BgLoop: resume from low-priority queue
    end
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• eloqdata/eloqstore#456: The PR directly implements the proposed solution—adding periodic cooperative yields inside long-running CPU-intensive loops via time-budgeted MaybeYield() to prevent shard worker thread stalls.

Possibly related PRs

• eloqdata/eloqstore#221: Modifies Shard::ExecuteReadyTasks() task yielding control flow, which is the same loop instrumented with cur_resume_start_us_ timestamps in this PR.
• eloqdata/eloqstore#293: Adds cooperative yielding inside compaction/file-GC loops to prevent worker-thread stalls using similar patterns to the time-budgeted yielding mechanism here.
• eloqdata/eloqstore#302: Implements YieldToLowPQ() and low_priority_ready_tasks_ infrastructure that MaybeYield() in this PR directly depends on for task suspension and resume.

Suggested reviewers

• liangjchen

🐇 A little timer ticks away,
Twenty microseconds, then yield the day!
Files and compaction loops must not overrun,
Chunked I/O keeps the shard worker's fun.
Manifests skipped, fdatasync kept with care —
The rabbit hops through checkpoints with flair! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 4.55% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check	❓ Inconclusive	Linked issue #39 (Stress test) lacks specific coding requirements, making it impossible to fully validate whether code changes meet explicit objectives from that issue.	Clarify whether stress test issue #39 contains specific coding requirements that should be validated against the implementation changes.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the main objective: reducing worker-thread occupancy from compaction/GC that stalls foreground serving.
Description check	✅ Passed	The description comprehensively explains the problem, three targeted changes, verification steps, and follow-up work, though it does not reference linked issues or RFC as specified in the template.
Out of Scope Changes check	✅ Passed	All code changes directly support the stated objectives: manifest replay optimization, io_uring chunking, and time-budgeted yielding for background compaction/GC.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch perf/compaction-gc-worker-occupation

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[liunyl]

we should have an overall yield budget option. This should apply to all sort of tasks. In fact I think we already have similar mechanism, yielding if task exceeds time limit. We should reuse that instead of defining a new one

[liunyl]

Done in 9026744. Renamed the flag to eloqstore_yield_budget_us (default lowered 500→20µs) and generalized it to all long-running task loops.

One nuance worth recording: I did look at reusing the existing time-limit mechanism (max_processing_time_microseconds), but it's a per-round budget — the scheduler only checks it in ExecuteReadyTasks between task resumes (shard.cpp:886/905), so it cannot preempt a single non-yielding coroutine segment, which is exactly the stall we're targeting. So an in-loop poll (MaybeYield) is still required; it can't be replaced by the round budget.

I kept eloqstore_yield_budget_us as a distinct per-segment budget and documented the difference next to the DEFINE. They can legitimately differ in value, and note max_processing_time_microseconds is UINT64_MAX (disabled) in debug builds whereas we want the segment yield active everywhere.

[liunyl]

again, should be a common function for all sort of long running loops. Not just for compaction

[liunyl]

Done in 9026744. Renamed MaybeYieldForCompaction → MaybeYield and generalized the doc comment (now: compaction, file GC, manifest replay, large scans, …).

Also replaced the adjacent count-based (round_cnt & 0xFF) coarse yields in DoCompactDataFile / DoCompactSegmentFile with the time-based MaybeYield() (and dropped the now-dead round_cnt), so those loops use the single self-tuning facility instead of two stacked idioms. The time budget self-tunes to actual segment cost, so it strictly subsumes the fixed 256-iteration yield.

[coderabbitai]

🧹 Nitpick comments (1)

src/file_gc.cpp (1)

730-770: 💤 Low value

Consider adding MaybeYieldForCompaction() in the archive manifest loop for consistency.

The regular manifest loop (line 673) has a yield call, but the archive loop does not. While the I/O operations naturally yield, the ProcessOneManifest call involves CPU-bound manifest replay that could be significant for large mappings. For consistency and to bound per-resume CPU time when many archives exist, consider adding a yield at the loop start.

♻️ Suggested change

     // --- Process archive manifests ---
     for (size_t i = 0; i < archive_files.size(); ++i)
     {
+        MaybeYieldForCompaction();
         const std::string &filename = archive_files[i];

🤖 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/file_gc.cpp` around lines 730 - 770, The archive manifest processing loop
that iterates through archive_files is missing a `MaybeYieldForCompaction()`
call at the loop start, which is inconsistent with the regular manifest loop and
does not bound CPU time for CPU-bound work in ProcessOneManifest. Add a
`MaybeYieldForCompaction()` call at the beginning of the for loop that iterates
through archive_files, before the filename extraction and file reading
operations, to ensure consistent yield behavior and bound per-resume CPU time
when processing multiple archives.

🤖 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.

Nitpick comments:
In `@src/file_gc.cpp`:
- Around line 730-770: The archive manifest processing loop that iterates
through archive_files is missing a `MaybeYieldForCompaction()` call at the loop
start, which is inconsistent with the regular manifest loop and does not bound
CPU time for CPU-bound work in ProcessOneManifest. Add a
`MaybeYieldForCompaction()` call at the beginning of the for loop that iterates
through archive_files, before the filename extraction and file reading
operations, to ensure consistent yield behavior and bound per-resume CPU time
when processing multiple archives.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8dad2101-33ae-4375-b753-1ec00608b49a

📥 Commits

Reviewing files that changed from the base of the PR and between 0fd2e4d and 66dade9.

📒 Files selected for processing (7)

• include/storage/shard.h
• include/tasks/task.h
• src/async_io_manager.cpp
• src/file_gc.cpp
• src/storage/shard.cpp
• src/tasks/background_write.cpp
• src/tasks/task.cpp