Add CPU scheduler - #86
Conversation
There was a problem hiding this comment.
🔵 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
SearchThreadPoolbacked by a newSchedulerthat 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.
There was a problem hiding this comment.
🟡 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 isscheduler_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
There was a problem hiding this comment.
🔵 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_searchspawns one call here for everyleaf_requestsentry (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 oneQueryIdfor 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_fieldstest, soprint_if_not_nullomits 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 isscheduler_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
Description
CPU scheduler to speed up small queries when the system is under heavy load.
Possible improvements:
How was this PR tested?
Not enough