`std::thread::scope()` improvements #98517

kprotty · 2022-06-26T02:06:11Z

avoids calling to Thread::unpark() opportunistically.
avoids calling thread::current() opportunistically.
properly pins ScopeData to indicate its on-stack nature.
outlines fast and slow paths for inc/dec/wait.
specializes on ThreadSanitizer for fences.
uses atomic intrinsics to avoid dangling shared references.

rust-highfive · 2022-06-26T02:06:14Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-06-26T02:06:15Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

Please see the contribution instructions for more information.

library/std/src/thread/scoped.rs

m-ou-se · 2022-06-27T14:29:55Z

properly pins ScopeData to indicate its on-stack nature.

What's the point of using Pin here? Pin is for things like self-referential structures, which may not be moved even if you do have a &mut to them. In this case, it may not be moved (or dropped) while it is borrowed, which is a rule that always applies to everything in Rust. That just requires &, with no need for Pin.

m-ou-se · 2022-06-27T15:15:45Z

avoids calling to Thread::unpark() opportunistically.

avoids calling thread::current() opportunistically.

This adds a lot of complexity, with no clear benefit. Do you have any benchmark or real-world use case where this matters?

m-ou-se · 2022-06-27T15:18:39Z

library/std/src/thread/scoped.rs

+    main_thread: Cell<Option<Thread>>,
+    _pinned: PhantomPinned,
 }

+unsafe impl Send for ScopeData {}
+unsafe impl Sync for ScopeData {}


Using Cell together with unsafe impl Sync is probably a bad idea. It removes all checks: any thread can now just call .get() and .set() on it, without any unsafe block.

All methods that operate on it are within the impl and main_thread cell accesses are commented as to who/when/how it's accessed. The Sync/Send are to get it to compile with Arc'ing of Packet internally.

Sure. I'm not saying that your code is unsound. I'm saying that it's very easy to make a mistake when forcing a Cell into a Sync type. We usually avoid that and keep using UnsafeCell to make sure we have to write unsafe when using it, so it can't accidentally be used from the wrong thread without thinking.

Oh, that makes sense.

kprotty · 2022-06-27T22:46:44Z

What's the point of using Pin here?

Pin is to indicate that it's pinned in memory somewhere (on the stack). It may not be moved/invalidated once it becomes pinned then dropped, not only while it is borrowed (that's Unpin as far as I know). I use it here for documentation purposes on spawn_unchecked_ to point out that it's meant to be stable/active/unmoved even after the call at the API boundary.

This adds a lot of complexity, with no clear benefit. Do you have any benchmark or real-world use case where this matters?

On macos, thread_local! is slower to access than other platforms. If the scopes complete in time, it may not be necessary to query TLS for current() or park(). This is also the point of the WAITING_BIT and the entire PR in general. If this optimization doesn't "matter" in practice, it can probably be closed.

m-ou-se · 2022-06-27T22:59:24Z

On macos, thread_local! is slower to access than other platforms. If the scopes complete in time, it may not be necessary to query TLS for current() or park(). This is also the point of the WAITING_BIT and the entire PR in general. If this optimization doesn't "matter" in practice, it can probably be closed.

Slower, but probably still insignificant compared to spawning new threads, I assume? I can see how this can make a significant difference for empty scopes where no threads are spawned, but I wonder if there are any realistic cases where this change makes a difference.

kprotty · 2022-06-27T23:08:58Z

You're right, TLS access is almost always faster than spawning threads on tier-1 and tier-2 platforms. It could maybe make a difference if the spawned threads completed before the scope completes or if no threads are ever spawned as mentioned.

thomcc · 2022-06-27T23:10:53Z

if the spawned threads completed before the scope completes

This does sound like a fairly common case (IME it is for my usage of crossbeam::scope anyway).

if no threads are ever spawned as mentioned.

OTOH this doesn't sound worth optimizing for.

bors · 2022-07-01T14:39:25Z

☔ The latest upstream changes (presumably #98730) made this pull request unmergeable. Please resolve the merge conflicts.

- avoids calling to Thread::unpark() opportunistically. - avoids calling thread::current() opportunistically. - properly pins ScopeData to indicate its on-stack nature. - outlines fast and slow paths for inc/dec/wait. - specializes on ThreadSanitizer for fences. - uses atomic intrinsics to avoid dangling shared references.

rustbot · 2022-07-02T02:54:39Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-log-analyzer · 2022-07-02T03:05:11Z

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling object v0.26.2
   Compiling std_detect v0.1.5 (/checkout/library/stdarch/crates/std_detect)
   Compiling hashbrown v0.12.0
   Compiling addr2line v0.16.0
error[E0432]: unresolved imports `core::intrinsics::atomic_store_rel`, `core::intrinsics::atomic_xsub_rel`
   |
   |
11 | use core::intrinsics::{atomic_store_rel, atomic_xsub_rel};
   |                        ^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^ no `atomic_xsub_rel` in `intrinsics`
   |                        |
   |                        no `atomic_store_rel` in `intrinsics`
help: a similar name exists in the module
   |
   |
11 | use core::intrinsics::{atomic_store_release, atomic_xsub_rel};
help: a similar name exists in the module
   |
   |
11 | use core::intrinsics::{atomic_store_rel, atomic_xsub_acqrel};

For more information about this error, try `rustc --explain E0432`.
error: could not compile `std` due to previous error
Build completed unsuccessfully in 0:05:00

bjorn3 · 2022-07-08T14:33:04Z

library/std/src/thread/scoped.rs

-    a_thread_panicked: AtomicBool,
-    main_thread: Thread,
+    sync_state: AtomicUsize,
+    thread_panicked: AtomicBool,


I think the previous name makes more sense. This is true when any thread panicked. thread_panicked to me suggests that there is only a single thread.

JohnCSimon · 2022-08-13T16:08:29Z

@kprotty
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rust-highfive assigned m-ou-se Jun 26, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 26, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2022

thomcc reviewed Jun 26, 2022

View reviewed changes

library/std/src/thread/scoped.rs Outdated Show resolved Hide resolved

crlf0710 changed the title ~~std::thread::scope() improvements~~ std::thread::scope() improvements Jun 26, 2022

m-ou-se reviewed Jun 27, 2022

View reviewed changes

m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 30, 2022

kprotty force-pushed the scoped-spawn branch from b523980 to 7ecb275 Compare July 2, 2022 02:54

bjorn3 reviewed Jul 8, 2022

View reviewed changes

JohnCSimon closed this Aug 13, 2022

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Aug 13, 2022

thomcc mentioned this pull request Sep 18, 2022

Scoped threads violate 'dereferenceable for function call' requirement of references #101983

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`std::thread::scope()` improvements #98517

`std::thread::scope()` improvements #98517

kprotty commented Jun 26, 2022

rust-highfive commented Jun 26, 2022

rust-highfive commented Jun 26, 2022

m-ou-se commented Jun 27, 2022

m-ou-se commented Jun 27, 2022

m-ou-se Jun 27, 2022

kprotty Jun 27, 2022

m-ou-se Jun 27, 2022

kprotty Jun 27, 2022

kprotty commented Jun 27, 2022

m-ou-se commented Jun 27, 2022

kprotty commented Jun 27, 2022

thomcc commented Jun 27, 2022

bors commented Jul 1, 2022

rustbot commented Jul 2, 2022

rust-log-analyzer commented Jul 2, 2022

bjorn3 Jul 8, 2022

JohnCSimon commented Aug 13, 2022

std::thread::scope() improvements #98517

std::thread::scope() improvements #98517

Conversation

kprotty commented Jun 26, 2022

rust-highfive commented Jun 26, 2022

rust-highfive commented Jun 26, 2022

m-ou-se commented Jun 27, 2022

m-ou-se commented Jun 27, 2022

m-ou-se Jun 27, 2022

Choose a reason for hiding this comment

kprotty Jun 27, 2022

Choose a reason for hiding this comment

m-ou-se Jun 27, 2022

Choose a reason for hiding this comment

kprotty Jun 27, 2022

Choose a reason for hiding this comment

kprotty commented Jun 27, 2022

m-ou-se commented Jun 27, 2022

kprotty commented Jun 27, 2022

thomcc commented Jun 27, 2022

bors commented Jul 1, 2022

rustbot commented Jul 2, 2022

rust-log-analyzer commented Jul 2, 2022

bjorn3 Jul 8, 2022

Choose a reason for hiding this comment

JohnCSimon commented Aug 13, 2022

`std::thread::scope()` improvements #98517

`std::thread::scope()` improvements #98517