Dynamic compute fns #2639
Dynamic compute fns #2639
Conversation
Deploying vortex-bench with
|
| Latest commit: |
71258c5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a07c0cbd.vortex-bench.pages.dev |
| Branch Preview URL: | https://ngates-dyn-compute-fns.vortex-bench.pages.dev |
CodSpeed Performance ReportMerging #2639 will improve performances by 13.27%Comparing Summary
Benchmarks breakdown
|
| const FILTER: Option<KernelRef> = None; | ||
|
|
||
| /// Fallback implementation to lookup compute kernels at runtime. | ||
| fn _find_kernel(&self, _compute_fn: &dyn ComputeFn) -> Option<KernelRef> { |
There was a problem hiding this comment.
don't you want to pass arguments here as well?
There was a problem hiding this comment.
Eh, not sure. Decided against because all kernels return Option<Output>. We may want to add later if a use-case comes up
| /// A trait used to register static kernels for known compute functions. | ||
| /// Dynamic kernels must be returned via the `_find_kernel` method. | ||
| pub trait ArrayComputeImpl { | ||
| const FILTER: Option<KernelRef> = None; |
There was a problem hiding this comment.
don't sure I understand how the default is supposed to work. I think you can explain a little bit more about the end state and it could make sense but sans commentary there's some weirdness here
There was a problem hiding this comment.
Basically the Impl things are just short-cuts to make implementing the Array trait easier. These consts allow you to return static lifetime kernels by wrapping up an impl FilterKernel.
The fallback to _find_kernel is in case an array wants to return dynamic kernels (i.e. Arc::new)
| // Check each of the known compute functions. | ||
|
|
||
| if any.is::<Filter>() { | ||
| return <Self as ArrayComputeImpl>::FILTER; |
There was a problem hiding this comment.
should this fallback as well? The default might not be populated
There was a problem hiding this comment.
What do you mean fall back? To what?
There was a problem hiding this comment.
to find_kernel if it's none
There was a problem hiding this comment.
That happens on line 88
There was a problem hiding this comment.
but this returns if the constant is none
There was a problem hiding this comment.
Oh gotcha, yep good catch
| use crate::arcref::ArcRef; | ||
| use crate::compute::{ComputeFn, InvocationArgs, Output}; | ||
|
|
||
| pub trait ComputeFnImpl { |
There was a problem hiding this comment.
TODO(ngates): remove this
Add the ability to define and dispatch compute at runtime.
Each compute function is now an
ArcRef<dyn ComputeFn>, these are no longer hard-coded and can be defined by users.The entry-point of a compute function can ask an array for a "kernel" for the specific compute function. This allows arrays to provide implementation kernels for any compute function, even those it doesn't have a compile-time dependency on.
It also allows arrays to define dynamic kernels runtime, for example, DictArray can define a single kernel to push-down compute for all element-wise operators.