Worker pool for current thread runtimes #5134
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Nicholas Gates <[email protected]>
Signed-off-by: Nicholas Gates <[email protected]>
Benchmarks: Random AccessSummary
|
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Benchmarks: FineWeb NVMeSummary
Detailed Results Table
|
Benchmarks: FineWeb S3Summary
Detailed Results Table
|
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
Benchmarks: CompressionSummary
Detailed Results Table
|
Benchmarks: GitHub Archive (S3)Summary
Detailed Results Table
|
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Benchmarks: GitHub Archive (NVMe)Summary
Detailed Results Table
|
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
robert3005
approved these changes
Nov 1, 2025
| // SPDX-FileCopyrightText: Copyright the Vortex contributors | ||
|
|
||
| #![allow(clippy::missing_safety_doc)] | ||
| // **WARNING end |
| // Note that this is optional and without it, we would only drive the task when DuckDB calls | ||
| // into us, and we call `RUNTIME.block_on`. | ||
| #[allow(dead_code)] | ||
| worker_pool: CurrentThreadWorkerPool, |
There was a problem hiding this comment.
We need to keep it alive as it holds onto the threads, but never need to do anything with it
robert3005
reviewed
Nov 2, 2025
| .name("vortex-current-thread-worker".to_string()) | ||
| .spawn(move || { | ||
| // Run the executor with a sleeping future that checks for shutdown | ||
| block_on(executor.run(async move { |
There was a problem hiding this comment.
The thing that wasn't immediately obvious to me is that this is not run this closure but run this executor until this future completes which maybe makes more sense if you have more rust runtime building experience. I.e. I thought this was spawn but there's spawn on this executor as well. This is just a comment for my understanding
robert3005
reviewed
Nov 2, 2025
| /// with the desired number of worker threads that will drive work on behalf of the runtime. | ||
| #[derive(Clone, Default)] | ||
| pub struct CurrentThreadRuntime { | ||
| executor: Arc<smol::Executor<'static>>, |
There was a problem hiding this comment.
is this a deliberate choice why this is not imported at the top level?
a10y
reviewed
Nov 3, 2025
| /// | ||
| /// By default, the pool has no worker threads; the caller must set the desired number of | ||
| /// worker threads using the `set_workers` method on the returned pool. | ||
| pub fn new_pool(&self) -> CurrentThreadWorkerPool { |
There was a problem hiding this comment.
what is the semantic of calling this method twice and having two CurrentThreadWorkerPools pointed at the same CurrentThreadRuntime?
Why isn't managment of workers part of the CurrentThreadRuntime?
There was a problem hiding this comment.
Both pools can make progress on the same execute. Agree it's a bit odd. Maybe it's worth just having a pool inside the runtime?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows current thread runtimes to have a dynamic number of background threads partaking in the work.
Removes the Tokio dependency from vortex-duckdb
Other language bindings should migrate to this runtime and expose a set_worker_threads(n) function to the language.