std::thread::scope() improvements

[kprotty]

• 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]

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]

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.

[m-ou-se]

• 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]

• 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]

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.

[kprotty]

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.

[m-ou-se]

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.

[kprotty]

Oh, that makes sense.

[kprotty]

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]

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]

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]

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]

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

[rustbot]

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]

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]

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]

@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