Implement in_place_scope
#844
Conversation
rocallahan
force-pushed
the
external-scope
branch
from
6fdb23f to
e9c138a
Compare
|
I fixed those check/format issues and repushed. |
rayon-core/src/scope/mod.rs
Outdated
| /// execute, even if the spawning task should later panic. `scope()` | ||
| /// returns once all spawned jobs have completed, and any panics are | ||
| /// propagated at that point. | ||
| pub fn external_scope<'scope, OP, R>(op: OP) -> R |
There was a problem hiding this comment.
I don't love the name 'external scope' -- it may not be external, after all, if this is running on a worker thread, right?
Maybe in_place_scope or something like that?
There was a problem hiding this comment.
in_place_scope sounds fine to me but @cuviper should weigh in I guess.
There was a problem hiding this comment.
direct_scope? immediate_scope? local_scope?
I'm not sure what will be most intuitive, that users might naturally guess what it means compared to scope alone.
(My own prior experimentation called it raw_scope, but I think that misses the mark.)
There was a problem hiding this comment.
this_thread_scope?
There was a problem hiding this comment.
in_place_scope or immediate_scope sound both good. The test is good to explain the use-case.
There was a problem hiding this comment.
@cuviper can you sign off on in_place_scope?
| @@ -218,6 +218,7 @@ impl<'r> Latch for SpinLatch<'r> { | |||
|
|
|||
| /// A Latch starts as false and eventually becomes true. You can block | |||
| /// until it becomes true. | |||
| #[derive(Debug)] | |||
rayon-core/src/scope/mod.rs
Outdated
| /// spawned into `s` complete. | ||
| /// | ||
| /// This is just like `scope()` except the closure runs on the same thread | ||
| /// that calls `external_scope()`. Only work that it spawns runs in the |
There was a problem hiding this comment.
Only calls to spawn()? Would be good to have the fifo variant at the same time?
There was a problem hiding this comment.
How about we get this reviewed and landed first so I don't duplicate any mistakes I've made?
rayon-core/src/scope/mod.rs
Outdated
| /// execute, even if the spawning task should later panic. `scope()` | ||
| /// returns once all spawned jobs have completed, and any panics are | ||
| /// propagated at that point. | ||
| pub fn external_scope<'scope, OP, R>(op: OP) -> R |
There was a problem hiding this comment.
in_place_scope or immediate_scope sound both good. The test is good to explain the use-case.
rocallahan
force-pushed
the
external-scope
branch
from
e9c138a to
bc3bb2f
Compare
|
I've uploaded new commits with the #[derive(Debug)] split out and the name changed to |
| @@ -329,6 +327,41 @@ impl AsCoreLatch for CountLatch { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Debug)] | |||
There was a problem hiding this comment.
Can you document the role of a "count lock latch", at least briefly?
| /// of the same registry as the scope itself! | ||
| Stealing { | ||
| latch: CountLatch, | ||
| registry: Arc<Registry>, |
There was a problem hiding this comment.
I'm curious what @cuviper thinks: I'd be inclined not to store this in a field but instead to pass an &Arc<Registry> as an argument to ScopeLatch::set. It avoids an extra atomic increment on every scope.
There was a problem hiding this comment.
What about the cross-registry case? If we call ThreadPool::in_place_scope() on registry B from a worker thread of registry A, when a job completes on a worker thread of registry B, we would need to call ScopeLatch::set with a reference to registry A but we won't be able to find one.
There was a problem hiding this comment.
I must not have my head in the right space; I don't quite follow. Maybe you can add a nice comment into the code detailing how that scenario plays out and why this field is necessary?
There was a problem hiding this comment.
I added a comment but I'm not sure I've made it any clearer than what I already said.
When a job completes in registry B we only have a reference to registry B, but we may need to wake the thread in registry A, for which we need a reference to registry A.
There was a problem hiding this comment.
Thanks. That helps. I'll try to make some time to read it over. I'm still contemplating what happened in the old code for this logic -- I guess the point is that this case didn't arise, because we always executed the scope's closure over within the target registry, so there was no cross-registry case? (Or at least, it was handled by the logic that moved over into the other registry?)
There was a problem hiding this comment.
I guess the point is that this case didn't arise, because we always executed the scope's closure over within the target registry, so there was no cross-registry case? (Or at least, it was handled by the logic that moved over into the other registry?)
Correct -- it would have ended up calling scope -> in_worker -> in_worker_cross to handle that case.
rocallahan
force-pushed
the
external-scope
branch
from
bc3bb2f to
b06e86f
Compare
|
I'm happy. I would be willing to merge. |
… same thread as the creator of the scope
|
bors r+ |
As discussed in #562.