Rename Shard tasks_to_run_next_round_ to low_priority_ready_tasks_ by xiexiaoy · Pull Request #302 · eloqdata/eloqstore

xiexiaoy · 2026-01-20T09:19:17Z

By renaming Shard::tasks_to_run_next_round_ to Shard::low_priority_ready_tasks_ and timing for CPU-heavy tasks, eloqstore yields brpc worker to other modules.

It should execute at least one CPU-heavy task.

max_concurrent_writes is removed to allow concurrent batchwrite.

Summary by CodeRabbit

New Features
- Added high-precision timing to improve task processing measurements.
Refactor
- Background work now yields to a low-priority queue to reduce interference with foreground tasks.
- Simplified write queuing by removing the configurable concurrent-write limit.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-20T09:19:24Z

Walkthrough

Renames task yield API to YieldToLowPQ and routes yields to a low-priority queue; removes KvOptions::max_concurrent_writes (INI load and equality checks); and adds Shard TSC/CNTVCT-based timing plus time-bounded processing for low-priority tasks.

Changes

Cohort / File(s)	Summary
Configuration & Options Removal `include/kv_options.h`, `src/kv_options.cpp`	Removed `uint32_t max_concurrent_writes` from `KvOptions`; removed INI parsing and equality comparison logic for that field.
Task / Yield API `include/tasks/task.h`, `src/tasks/task.cpp`	Renamed `YieldToNextRound()` → `YieldToLowPQ()`; enqueue destination changed from `tasks_to_run_next_round_` to `low_priority_ready_tasks_`.
Shard timing & queue management `include/storage/shard.h`, `src/storage/shard.cpp`	Added TSC/CNTVCT timing helpers and static frequency state (`InitializeTscFrequency()`, `ReadTimeMicroseconds()`, `DurationMicroseconds()`, `tsc_frequency_initialized_`, `tsc_cycles_per_microsecond_`); renamed queue to `low_priority_ready_tasks_`; ExecuteReadyTasks() enforces a time budget.
Yield call-site updates `src/file_gc.cpp`, `src/storage/page_mapper.cpp`, `src/storage/root_meta.cpp`, `src/tasks/background_write.cpp`, `src/tasks/write_task.cpp`	Replaced calls to `YieldToNextRound()` with `YieldToLowPQ()` across GC, mapping, compaction, and write paths.

Sequence Diagram(s)

sequenceDiagram
    participant Task as KvTask
    participant Shard as Shard
    participant Clock as TSC/Clock
    participant Queue as low_priority_ready_tasks_

    Note over Task,Shard: Yielding and time-bounded resume flow
    Task->>Shard: YieldToLowPQ() (enqueue self)
    Shard->>Queue: Enqueue(KvTask)
    Shard->>Clock: ReadTimeMicroseconds()
    Clock-->>Shard: current_us
    Shard->>Shard: ExecuteReadyTasks(start_us=current_us)
    loop while DurationMicroseconds(start_us) < max_processing_time_microseconds
        Shard->>Queue: Dequeue() -> KvTask
        Shard-->>Task: Resume task
        Shard->>Clock: ReadTimeMicroseconds()
        Clock-->>Shard: current_us
    end

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly Related PRs

Insert YieldToNextRound in compaction to avoid affecting online tasks #293: Modifies the same yield points in compaction and background-task code; directly related.
Revert "Introduce arena for mapping and add YieldToNextRound() to make WorkOneRound() faster" #221: Changes task-yielding API and overlaps with the removal/rename of YieldToNextRound.
Introduce arena for mapping and add YieldToNextRound() to make WorkOneRound() faster #209: Earlier work on cooperative-yield API that this PR renames/refactors.

Suggested Reviewers

thweetkomputer
eatbreads

Poem

🐇 I hopped from round to low-priority queues,
I traded ticks for tiny timed views,
I queued my task with a twitch and a bow,
The shard keeps time — I pause, then I plow,
A rabbit’s dance in code and muse.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description covers the main objectives but is missing required documentation from the template checklist. Most required sections are not addressed.	Complete the PR description template by adding checkboxes for tests, documentation, issue references, RFC links, and confirming ctest passes.
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the primary structural change across the codebase: renaming the tasks queue and related method from YieldToNextRound to YieldToLowPQ, which is the main refactoring.

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

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch low_pq

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

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

CLAassistant · 2026-01-20T09:19:30Z

All committers have signed the CLA.

liunyl · 2026-01-22T07:44:30Z

src/storage/shard.cpp

    {
-        KvTask *task = tasks_to_run_next_round_.Peek();
-        tasks_to_run_next_round_.Dequeue();
+        KvTask *task = low_priority_ready_tasks_.Peek();


why is low prio tasks re-enqueued to ready tasks?

liunyl · 2026-01-22T07:44:47Z

src/storage/shard.cpp

-    while (tasks_to_run_next_round_.Size() > 0)
+
+    uint64_t delta_us = DurationMicroseconds(start_us_fast);
+    while (delta_us < MAX_PROCESSING_TIME_MICROSECONDS &&


we should allow at least 1 low prio task to be executed per round.

Make MAX_PROCESSING_TIME_MICROSECONDS a brpc flag

we should allow at least 1 low prio task to be executed per round.

Fixed

github-actions · 2026-01-22T08:07:15Z

src/storage/shard.cpp

+    return ticks / cycles_per_us;
+#else
+    // Fallback to std::chrono (slower but portable and precise)
+    using namespace std::chrono;


[cpplint] _{reported by reviewdog 🐶}
Do not use namespace using-directives. Use using-declarations instead. [build/namespaces] [5]

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/storage/shard.cpp`:
- Around line 793-806: The DurationMicroseconds implementation misclassifies
end_us == start_us as wraparound; update Shard::DurationMicroseconds to treat
equal timestamps as zero elapsed by changing the comparison to use >= (i.e., if
end_us >= start_us return end_us - start_us) and only return
FLAGS_max_processing_time_microseconds when end_us < start_us (actual
wraparound) so low‑priority work isn't cut prematurely.

coderabbitai · 2026-01-22T08:11:28Z

src/storage/shard.cpp

+uint64_t Shard::DurationMicroseconds(uint64_t start_us)
+{
+    // Check elapsed time (returns microseconds directly)
+    uint64_t end_us = ReadTimeMicroseconds();
+    // Handle potential wraparound (unlikely but safe)
+    if (end_us > start_us)
+    {
+        return end_us - start_us;
+    }
+    else
+    {
+        // Wraparound detected, use max value to break loop
+        return FLAGS_max_processing_time_microseconds;
+    }


⚠️ Potential issue | 🟡 Minor

Fix zero‑delta handling in DurationMicroseconds.
When end_us == start_us, the current > check treats it as wraparound, returning the max and prematurely cutting low‑priority work.

🛠️ Proposed fix

- if (end_us > start_us) + if (end_us >= start_us) { return end_us - start_us; }

🤖 Prompt for AI Agents

In `@src/storage/shard.cpp` around lines 793 - 806, The DurationMicroseconds implementation misclassifies end_us == start_us as wraparound; update Shard::DurationMicroseconds to treat equal timestamps as zero elapsed by changing the comparison to use >= (i.e., if end_us >= start_us return end_us - start_us) and only return FLAGS_max_processing_time_microseconds when end_us < start_us (actual wraparound) so low‑priority work isn't cut prematurely.

xiexiaoy force-pushed the low_pq branch 6 times, most recently from d701ac8 to 669176b Compare January 22, 2026 07:13

xiexiaoy added the ci-approved label Jan 22, 2026

xiexiaoy marked this pull request as ready for review January 22, 2026 07:14

xiexiaoy mentioned this pull request Jan 22, 2026

eloqstore: remove write limit eloqdata/tx_service#375

Merged

xiexiaoy requested a review from liunyl January 22, 2026 07:18

xiexiaoy linked an issue Jan 22, 2026 that may be closed by this pull request

[Feature]: Remove limitation of cloud task from write task #298

Closed

github-actions bot removed the ci-approved label Jan 22, 2026

liunyl reviewed Jan 22, 2026

View reviewed changes

xiexiaoy added the ci-approved label Jan 22, 2026

xiexiaoy force-pushed the low_pq branch from f3a8dde to 67d229e Compare January 22, 2026 08:06

github-actions bot reviewed Jan 22, 2026

View reviewed changes

github-actions bot removed the ci-approved label Jan 22, 2026

coderabbitai bot reviewed Jan 22, 2026

View reviewed changes

xiexiaoy requested a review from liunyl January 22, 2026 08:18

This was referenced Jan 22, 2026

eloqstore: remove concurrent write limit eloqdata/eloqkv#385

Merged

eloqstore: remove concurrent write limit eloqdata/eloqdoc#439

Merged

eloqstore: remove concurrent write limit eloqdata/eloqsql#219

Merged

xiexiaoy added 5 commits January 22, 2026 17:40

Rename Shard tasks_to_run_next_round_ to low_priority_ready_tasks_

293c67e

debug log

e13b8ca

Remove log

09acee5

Fix format

e2c3309

Execute at least one low priority task

c81515e

xiexiaoy force-pushed the low_pq branch from 67d229e to c81515e Compare January 22, 2026 09:40

liunyl approved these changes Jan 22, 2026

View reviewed changes

xiexiaoy merged commit ce2cd44 into main Jan 22, 2026
4 checks passed

xiexiaoy deleted the low_pq branch January 22, 2026 09:56

This was referenced Jan 28, 2026

feat: reduce low-tail latency via root-meta cache and IO tuning #320

Closed

feat: introduce merge write and use fixed IO and direct fd to cut low-tail latency #321

Merged

coderabbitai bot mentioned this pull request Feb 13, 2026

Fix test zc #365

Closed

5 tasks

coderabbitai bot mentioned this pull request Feb 26, 2026

fix: call io_mgr::stop in module thread when stopping #381

Merged

5 tasks

coderabbitai bot mentioned this pull request Mar 11, 2026

Async sealed upload #405

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename Shard tasks_to_run_next_round_ to low_priority_ready_tasks_#302

Rename Shard tasks_to_run_next_round_ to low_priority_ready_tasks_#302
xiexiaoy merged 5 commits intomainfrom
low_pq

xiexiaoy commented Jan 20, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jan 20, 2026 •

edited

Loading

Uh oh!

CLAassistant commented Jan 20, 2026 •

edited

Loading

Uh oh!

liunyl Jan 22, 2026

Uh oh!

liunyl Jan 22, 2026

Uh oh!

liunyl Jan 22, 2026

Uh oh!

xiexiaoy Jan 22, 2026

Uh oh!

github-actions bot Jan 22, 2026

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Jan 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

xiexiaoy commented Jan 20, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated Code Review Effort

Possibly Related PRs

Suggested Reviewers

Poem

Uh oh!

CLAassistant commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liunyl Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

liunyl Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

liunyl Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

xiexiaoy Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xiexiaoy commented Jan 20, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 20, 2026 •

edited

Loading

CLAassistant commented Jan 20, 2026 •

edited

Loading