Skip to content

Add CPU scheduler - #86

Open
rdettai-sk wants to merge 7 commits into
sekoiafrom
cpu-scheduler
Open

Add CPU scheduler#86
rdettai-sk wants to merge 7 commits into
sekoiafrom
cpu-scheduler

Conversation

@rdettai-sk

Copy link
Copy Markdown
Collaborator

Description

CPU scheduler to speed up small queries when the system is under heavy load.

Possible improvements:

  • replace set_waiting_for_permit with a shared atomic to decrease lock contention

How was this PR tested?

Not enough

Copilot AI lite review requested due to automatic review settings September 3, 2026 20:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It adds a new concurrency scheduler layer and currently has correctness issues (panic accounting and zero-split registration leak) with limited end-to-end coverage.

Pull request overview

This PR introduces a per-query fair-share CPU scheduler in quickwit-common and wires it into quickwit-search so split-level CPU work can be scheduled more fairly under load while keeping merge/finalize work high-priority.

Changes:

  • Added SearchThreadPool backed by a new Scheduler that supports per-query fair-share scheduling plus a high-priority FIFO lane.
  • Updated leaf search to register queries/splits and run split CPU work via run_cpu_intensive_fair(query_id, ...).
  • Migrated existing “merge/finalize” CPU work to SearchThreadPool::run_cpu_intensive(...) (high-priority FIFO) and refactored thread-pool code into a module layout.
File summaries
File Description
quickwit/quickwit-search/src/root.rs Uses the new SearchThreadPool::run_cpu_intensive for root merge CPU work.
quickwit/quickwit-search/src/list_fields.rs Uses the new SearchThreadPool::run_cpu_intensive for list-fields merge CPU work.
quickwit/quickwit-search/src/lib.rs Switches the search pool singleton type from ThreadPool to SearchThreadPool.
quickwit/quickwit-search/src/leaf.rs Registers per-query scheduling state and routes split CPU work through fair scheduling.
quickwit/quickwit-common/src/thread_pool/mod.rs Introduces the new thread-pool module structure and re-exports.
quickwit/quickwit-common/src/thread_pool/regular_pool.rs Provides the baseline FIFO ThreadPool implementation (refactor of old code).
quickwit/quickwit-common/src/thread_pool/search_pool.rs Adds SearchThreadPool wrapper with FIFO and fair-share entrypoints.
quickwit/quickwit-common/src/thread_pool/scheduler.rs Implements the new priority + per-query fair-share scheduler and unit tests.
quickwit/quickwit-common/src/thread_pool.rs Removes the previous monolithic thread_pool implementation in favor of the module split.
Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 5
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread quickwit/quickwit-common/src/thread_pool/scheduler.rs
Comment thread quickwit/quickwit-common/src/thread_pool/scheduler.rs
Comment thread quickwit/quickwit-common/src/thread_pool/regular_pool.rs Outdated
Comment thread quickwit/quickwit-common/src/thread_pool/regular_pool.rs
Comment thread quickwit/quickwit-common/src/thread_pool/scheduler.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new cancellation outcome is lost during response merging, and the yield regression test does not reliably exercise starvation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

quickwit/quickwit-common/src/thread_pool/scheduler.rs:585

  • This test can pass without the yield path: 2,000 one-millisecond jobs on two workers drain in roughly one second, while the direct task is allowed two seconds. Make the backlog unable to drain before the timeout (for example, sustain it with synchronization) so removing the periodic yield reliably fails the test.
    quickwit/quickwit-search/src/leaf.rs:1583
  • This comment names _split_guard, but the field added below is scheduler_guard; update it so the cleanup mechanism points to the actual guard.
  • Files reviewed: 14/14 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread quickwit/quickwit-proto/protos/quickwit/search.proto
@rdettai-sk
rdettai-sk requested review from Darkheir and a balanced review from Copilot September 4, 2026 13:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Multi-index requests receive multiple scheduler shares, undermining the intended per-query fairness.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

quickwit/quickwit-search/src/leaf.rs:1415

  • This creates one scheduler identity per index/doc-mapping child rather than per leaf request. multi_index_leaf_search spawns one call here for every leaf_requests entry (lines 1285-1325), so a single multi-index request receives multiple independent fair-share slots and can occupy the pool ahead of a genuinely small request. Register one QueryId for the whole incoming leaf request and distribute its split guards to the child searches.

This issue also appears on line 1583 of the same file.
quickwit/quickwit-search/src/metrics_trackers.rs:337

  • The newly added field is zero in the all_fields test, so print_if_not_null omits it and the new display branch is never verified. Give it a non-zero value and include it in the expected output.

quickwit/quickwit-search/src/leaf.rs:1583

  • This comment names _split_guard, but the field introduced below is scheduler_guard. Refer to the actual field so the cleanup mechanism is clear.
        // Resolving the split with the scheduler happens as `_split_guard` drops.
  • Files reviewed: 14/14 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants