Conversation
github-actions
bot
removed
the
action/benchmark
Contributor
Benchmarks: TPC-HTable of Results
|
Contributor
Benchmarks: random_access |
CodSpeed Performance ReportMerging #2348 will improve performances by 43.48%Comparing Summary
Benchmarks breakdown
|
AdamGS
commented
Feb 13, 2025
vortex-layout/src/scan/task.rs
Outdated
| #[async_trait] | ||
| impl TaskExecutor for InlineTaskExecutor { | ||
| async fn execute(&self, array: &Array, tasks: &[ScanTask]) -> VortexResult<Array> { | ||
| fn execute(&self, array: &Array, tasks: &[ScanTask]) -> VortexResult<Array> { |
Contributor
Author
There was a problem hiding this comment.
this is a flyby but seems like it can be blocking now? potentially not even needed but I'm not sure what are your plans for it
github-actions
bot
removed
the
action/benchmark
Contributor
Benchmarks: compressTable of Results
|
Contributor
Author
|
made another round of changes, I think this interface is more general (and works better for the non-specific runtime case), but |
Contributor
Author
|
I have an idea for the non-specific-runtime variant, coding it now |
AdamGS
commented
Feb 14, 2025
| result: oneshot::Sender<R>, | ||
| } | ||
|
|
||
| impl<F, R> Task for ExecutorTask<F, R> |
AdamGS
added a commit
that referenced
this pull request
Feb 14, 2025
just a temporary measure until the [language API](rust-lang/rust#70142) is stabilized. I have a couple of these cases in #2348 so I figured its worth pulling into a separate PR.
AdamGS
added
action/benchmark
github-actions
bot
removed
the
action/benchmark-sql
Contributor
Benchmarks: TPC-H on NVMETable of Results
|
AdamGS
commented
Feb 18, 2025
| } | ||
| } | ||
|
|
||
| struct Inner { |
Contributor
Author
There was a problem hiding this comment.
This structure is needed to achieve the behavior we want, where arbitrary futures can be spawned on this runtime. I've tried to structure it differently but I honestly think this is pretty nice, and we can definitely try and improve that later.
github-actions
bot
removed
the
action/benchmark-sql
Contributor
Benchmarks: TPC-H on S3Table of Results
|
Contributor
Benchmarks: Clickbench on NVMETable of Results
|
Contributor
Benchmarks: TPC-H on NVMETable of Results
|
Contributor
Author
|
All benchmarks look good, not much difference on S3 (maybe slightly better) and much better on disk (especially for TPC-H). |
gatesn
reviewed
Feb 18, 2025
vortex-layout/src/scan/mod.rs
Outdated
| split_by: SplitBy::Layout, | ||
| canonicalize: false, | ||
| concurrency: 32, | ||
| concurrency: std::thread::available_parallelism() |
Contributor
There was a problem hiding this comment.
I don't know that this should default to nthreads, this is "concurrency" and not "parallelism", the number of row splits to make progress on concurrently. Basically... we want as many as possible provided the data fits in some reasonable memory, since the more concurrency, the more visibility into upcoming I/O, and therefore the more coalescing.
Contributor
Author
There was a problem hiding this comment.
yeah, it was an idea but we probably do want to oversubscribe the tokio runtime if we can and buffer by a different metric
| use vortex_error::VortexResult; | ||
|
|
||
| pub trait Executor { | ||
| /// Spawns a future to run on a different runtime. The shouldn't be polled to make progress. |
Contributor
Author
There was a problem hiding this comment.
LMK what you think of the new one
|
|
||
| pub trait Executor { | ||
| /// Spawns a future to run on a different runtime. The shouldn't be polled to make progress. | ||
| fn spawn<F>(&self, f: F) -> VortexResult<BoxFuture<'static, VortexResult<F::Output>>> |
Contributor
There was a problem hiding this comment.
Does this need to return a result? Can it just be BoxFuture<'static, VortexResult<..>>? Since the impl can always return ready(Err(...)) if it needs to.
Contributor
Author
There was a problem hiding this comment.
I don' think we want to return ready(Err(...)) here, but if we're ok with panicking in cases where we can't submit work/tasks start getting cancelled, we can get the nicer function signature.
|
|
||
| /// Generic wrapper around different async runtimes. Used to spawn async tasks to run in the background, concurrently with other tasks. | ||
| #[derive(Clone)] | ||
| pub enum TaskExecutor { |
Contributor
There was a problem hiding this comment.
I wonder what the performance hit is to just have a dyn Executor? Seems like a lot of code here for basically |f| tokio::spawn(f)
Contributor
Author
There was a problem hiding this comment.
IDK if we can have dyn Executor here without a major change due to the generics
Contributor
There was a problem hiding this comment.
You would have to take / return BoxFuture. You already return one
Contributor
Author
There was a problem hiding this comment.
I don't think its that much code (probably less than what happens in Arc), but in order to make it dyn-compatible we'll also have to move the generic bound from the function to the trait itself (example), which makes this thing less useful in other cases IMO.
github-actions
bot
removed
the
action/benchmark-sql
gatesn
reviewed
Feb 18, 2025
gatesn
approved these changes
Feb 18, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
very hacky demo