-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for bulk spawn. #1420
base: main
Are you sure you want to change the base?
Conversation
The first question that popped in my mind while starting my review is: why do we need a special type to represent a collection of futures? Is there a way we could use |
We can, but that would be inefficient. We would essentially be needing escaping futures, and thus we have one memory allocation for each computation. Using |
@@ -0,0 +1,96 @@ | |||
/// A future that is obtained for a bulk spawn. | |||
public type BulkFuture<E: Movable & Deinitializable> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public type BulkFuture<E: Movable & Deinitializable> { | |
public type FutureSet<E: Movable & Deinitializable> { |
or
public type BulkFuture<E: Movable & Deinitializable> { | |
public type FutureArray<E: Movable & Deinitializable> { |
?
The term "bulk" does not work for me. Even if we can't actually use Set
, Array
, or Buffer
, at least we can probably relate this abstraction to one of these collection using its name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an array of futures or a set of futures, but I see why you don't like it. It a future that waits on multiple actions to complete.
I'm thinking at this entire thing as supporting "bulk spawn"; initially, I had the name bulk_spawn_
, and only later renamed it to spawn_
. This is why the future still has "bulk" in it.
I would need to think some more about better naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to FutureSet
.
/// The underlying frame object that is used to connect the spawned computation with the await point. | ||
private var frame: PointerToMutable<BulkSpawnFrame<E>> | ||
|
||
/// Initializes `self` with `f`, and spawns the computation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Initializes `self` with `f`, and spawns the computation. | |
/// Initializes `self` with `action`, and spawns the computation. |
I typically try to describe how a callback is called in the docs. We should know what index
is and what values we can expect receiving in the lambda that we pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
/// Awaits the result of the computation. | ||
/// | ||
/// - Note: May return on a different OS thread than the one that called this. | ||
public fun await() sink { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little surprised that this doesn't return anything. Is it because of current compiler limitations?
I would have expected to get a set/array/buffer out of the await
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can return something like an array. Bur, I would like to avoid another allocation and I would wish these elements to reside in the frame. Thus, we need to return a Collection
type (random access) that is able to query the frame for the results.
But I would add this later, when we add more meat on the standard library.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
We can spawn multiple computation in parallel, at the same time.