Skip to content
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

Parallel-compiler-related cleanup #136858

Merged
merged 11 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions compiler/rustc_codegen_gcc/src/back/lto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -710,10 +710,6 @@ pub struct ThinBuffer {
context: Arc<SyncContext>,
}

// TODO: check if this makes sense to make ThinBuffer Send and Sync.
unsafe impl Send for ThinBuffer {}
unsafe impl Sync for ThinBuffer {}

impl ThinBuffer {
pub(crate) fn new(context: &Arc<SyncContext>) -> Self {
Self { context: Arc::clone(context) }
Expand Down
3 changes: 0 additions & 3 deletions compiler/rustc_codegen_llvm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,6 @@ impl WriteBackendMethods for LlvmCodegenBackend {
}
}

unsafe impl Send for LlvmCodegenBackend {} // Llvm is on a per-thread basis
unsafe impl Sync for LlvmCodegenBackend {}

impl LlvmCodegenBackend {
pub fn new() -> Box<dyn CodegenBackend> {
Box::new(LlvmCodegenBackend(()))
Expand Down
15 changes: 5 additions & 10 deletions compiler/rustc_data_structures/src/owned_slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,6 @@ use std::borrow::Borrow;
use std::ops::Deref;
use std::sync::Arc;

// Use our fake Send/Sync traits when on not parallel compiler,
// so that `OwnedSlice` only implements/requires Send/Sync
// for parallel compiler builds.
use crate::sync;

/// An owned slice.
///
/// This is similar to `Arc<[u8]>` but allows slicing and using anything as the
Expand Down Expand Up @@ -34,7 +29,7 @@ pub struct OwnedSlice {
// \/
// ⊂(´・◡・⊂ )∘˚˳° (I am the phantom remnant of #97770)
#[expect(dead_code)]
owner: Arc<dyn sync::Send + sync::Sync>,
owner: Arc<dyn Send + Sync>,
}

/// Makes an [`OwnedSlice`] out of an `owner` and a `slicer` function.
Expand All @@ -61,7 +56,7 @@ pub struct OwnedSlice {
/// ```
pub fn slice_owned<O, F>(owner: O, slicer: F) -> OwnedSlice
where
O: sync::Send + sync::Sync + 'static,
O: Send + Sync + 'static,
F: FnOnce(&O) -> &[u8],
{
try_slice_owned(owner, |x| Ok::<_, !>(slicer(x))).into_ok()
Expand All @@ -72,7 +67,7 @@ where
/// See [`slice_owned`] for the infallible version.
pub fn try_slice_owned<O, F, E>(owner: O, slicer: F) -> Result<OwnedSlice, E>
where
O: sync::Send + sync::Sync + 'static,
O: Send + Sync + 'static,
F: FnOnce(&O) -> Result<&[u8], E>,
{
// We wrap the owner of the bytes in, so it doesn't move.
Expand Down Expand Up @@ -139,10 +134,10 @@ impl Borrow<[u8]> for OwnedSlice {
}

// Safety: `OwnedSlice` is conceptually `(&'self.1 [u8], Arc<dyn Send + Sync>)`, which is `Send`
unsafe impl sync::Send for OwnedSlice {}
unsafe impl Send for OwnedSlice {}

// Safety: `OwnedSlice` is conceptually `(&'self.1 [u8], Arc<dyn Send + Sync>)`, which is `Sync`
unsafe impl sync::Sync for OwnedSlice {}
unsafe impl Sync for OwnedSlice {}

#[cfg(test)]
mod tests;
43 changes: 3 additions & 40 deletions compiler/rustc_data_structures/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,8 @@
//!
//! | Type | Serial version | Parallel version |
//! | ----------------------- | ------------------- | ------------------------------- |
//! |` Weak<T>` | `rc::Weak<T>` | `sync::Weak<T>` |
//! | `LRef<'a, T>` [^2] | `&'a mut T` | `&'a T` |
//! | | | |
//! | `AtomicBool` | `Cell<bool>` | `atomic::AtomicBool` |
//! | `AtomicU32` | `Cell<u32>` | `atomic::AtomicU32` |
//! | `AtomicU64` | `Cell<u64>` | `atomic::AtomicU64` |
//! | `AtomicUsize` | `Cell<usize>` | `atomic::AtomicUsize` |
//! | | | |
//! | `Lock<T>` | `RefCell<T>` | `RefCell<T>` or |
//! | | | `parking_lot::Mutex<T>` |
//! | `RwLock<T>` | `RefCell<T>` | `parking_lot::RwLock<T>` |
Expand Down Expand Up @@ -103,18 +97,15 @@ mod mode {

// FIXME(parallel_compiler): Get rid of these aliases across the compiler.

pub use std::marker::{Send, Sync};
pub use std::sync::OnceLock;
// Use portable AtomicU64 for targets without native 64-bit atomics
#[cfg(target_has_atomic = "64")]
pub use std::sync::atomic::AtomicU64;
pub use std::sync::atomic::{AtomicBool, AtomicU32, AtomicUsize};
pub use std::sync::{OnceLock, Weak};

pub use mode::{is_dyn_thread_safe, set_dyn_thread_safe_mode};
pub use parking_lot::{
MappedMutexGuard as MappedLockGuard, MappedRwLockReadGuard as MappedReadGuard,
MappedRwLockWriteGuard as MappedWriteGuard, RwLockReadGuard as ReadGuard,
RwLockWriteGuard as WriteGuard,
MappedRwLockReadGuard as MappedReadGuard, MappedRwLockWriteGuard as MappedWriteGuard,
RwLockReadGuard as ReadGuard, RwLockWriteGuard as WriteGuard,
};
#[cfg(not(target_has_atomic = "64"))]
pub use portable_atomic::AtomicU64;
Expand Down Expand Up @@ -203,12 +194,6 @@ impl<T> RwLock<T> {
}
}

#[inline(always)]
#[track_caller]
pub fn with_read_lock<F: FnOnce(&T) -> R, R>(&self, f: F) -> R {
f(&*self.read())
}

#[inline(always)]
pub fn try_write(&self) -> Result<WriteGuard<'_, T>, ()> {
self.0.try_write().ok_or(())
Expand All @@ -223,12 +208,6 @@ impl<T> RwLock<T> {
}
}

#[inline(always)]
#[track_caller]
pub fn with_write_lock<F: FnOnce(&mut T) -> R, R>(&self, f: F) -> R {
f(&mut *self.write())
}

#[inline(always)]
#[track_caller]
pub fn borrow(&self) -> ReadGuard<'_, T> {
Expand All @@ -240,20 +219,4 @@ impl<T> RwLock<T> {
pub fn borrow_mut(&self) -> WriteGuard<'_, T> {
self.write()
}

#[inline(always)]
pub fn leak(&self) -> &T {
let guard = self.read();
let ret = unsafe { &*(&raw const *guard) };
std::mem::forget(guard);
ret
}
}

// FIXME: Probably a bad idea
impl<T: Clone> Clone for RwLock<T> {
#[inline]
fn clone(&self) -> Self {
RwLock::new(self.borrow().clone())
}
}
4 changes: 2 additions & 2 deletions compiler/rustc_data_structures/src/sync/freeze.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ use std::intrinsics::likely;
use std::marker::PhantomData;
use std::ops::{Deref, DerefMut};
use std::ptr::NonNull;
use std::sync::atomic::Ordering;
use std::sync::atomic::{AtomicBool, Ordering};

use crate::sync::{AtomicBool, DynSend, DynSync, ReadGuard, RwLock, WriteGuard};
use crate::sync::{DynSend, DynSync, ReadGuard, RwLock, WriteGuard};

/// A type which allows mutation using a lock until
/// the value is frozen and can be accessed lock-free.
Expand Down
11 changes: 5 additions & 6 deletions compiler/rustc_data_structures/src/sync/worker_local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,6 @@ pub struct WorkerLocal<T> {
registry: Registry,
}

// This is safe because the `deref` call will return a reference to a `T` unique to each thread
// or it will panic for threads without an associated local. So there isn't a need for `T` to do
// it's own synchronization. The `verify` method on `RegistryId` has an issue where the id
// can be reused, but `WorkerLocal` has a reference to `Registry` which will prevent any reuse.
unsafe impl<T: Send> Sync for WorkerLocal<T> {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this impl removed?

Copy link
Member

@SparrowLii SparrowLii Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it should be keeped. The design of WokerLocal should return references to variables by thread id as indexes.
Thanks and sorry for the missing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@safinaskar Can you submit a follow-up PR to revert the changes to WorkerLocak here?


impl<T> WorkerLocal<T> {
/// Creates a new worker local where the `initial` closure computes the
/// value this worker local should take for each thread in the registry.
Expand All @@ -138,6 +132,11 @@ impl<T> Deref for WorkerLocal<T> {
fn deref(&self) -> &T {
// This is safe because `verify` will only return values less than
// `self.registry.thread_limit` which is the size of the `self.locals` array.

// The `deref` call will return a reference to a `T` unique to each thread
// or it will panic for threads without an associated local. So there isn't a need for `T` to do
// it's own synchronization. The `verify` method on `RegistryId` has an issue where the id
// can be reused, but `WorkerLocal` has a reference to `Registry` which will prevent any reuse.
unsafe { &self.locals.get_unchecked(self.registry.id().verify()).0 }
}
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_query_system/src/dep_graph/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ use std::fmt::Debug;
use std::hash::Hash;
use std::marker::PhantomData;
use std::sync::Arc;
use std::sync::atomic::Ordering;
use std::sync::atomic::{AtomicU32, Ordering};

use rustc_data_structures::fingerprint::Fingerprint;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::profiling::{QueryInvocationId, SelfProfilerRef};
use rustc_data_structures::sharded::{self, Sharded};
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_data_structures::sync::{AtomicU32, AtomicU64, Lock};
use rustc_data_structures::sync::{AtomicU64, Lock};
use rustc_data_structures::unord::UnordMap;
use rustc_index::IndexVec;
use rustc_macros::{Decodable, Encodable};
Expand Down
5 changes: 0 additions & 5 deletions config.example.toml
Original file line number Diff line number Diff line change
Expand Up @@ -595,11 +595,6 @@
# Whether to always use incremental compilation when building rustc
#incremental = false

# Build a multi-threaded rustc. This allows users to use parallel rustc
# via the unstable option `-Z threads=n`.
# This option is deprecated and always true.
#parallel-compiler = true

# The default linker that will be hard-coded into the generated
# compiler for targets that don't specify a default linker explicitly
# in their target specifications. Note that this is not the linker
Expand Down
5 changes: 0 additions & 5 deletions src/doc/rustc-dev-guide/src/parallel-rustc.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ are implemented differently depending on whether `parallel-compiler` is true.

| data structure | parallel | non-parallel |
| -------------------------------- | --------------------------------------------------- | ------------ |
| Weak | std::sync::Weak | std::rc::Weak |
| Atomic{Bool}/{Usize}/{U32}/{U64} | std::sync::atomic::Atomic{Bool}/{Usize}/{U32}/{U64} | (std::cell::Cell<bool/usize/u32/u64>) |
| OnceCell | std::sync::OnceLock | std::cell::OnceCell |
| Lock\<T> | (parking_lot::Mutex\<T>) | (std::cell::RefCell) |
| RwLock\<T> | (parking_lot::RwLock\<T>) | (std::cell::RefCell) |
Expand All @@ -58,7 +56,6 @@ are implemented differently depending on whether `parallel-compiler` is true.
| WriteGuard | parking_lot::RwLockWriteGuard | std::cell::RefMut |
| MappedWriteGuard | parking_lot::MappedRwLockWriteGuard | std::cell::RefMut |
| LockGuard | parking_lot::MutexGuard | std::cell::RefMut |
| MappedLockGuard | parking_lot::MappedMutexGuard | std::cell::RefMut |

- These thread-safe data structures are interspersed during compilation which
can cause lock contention resulting in degraded performance as the number of
Expand Down Expand Up @@ -173,12 +170,10 @@ Here are some resources that can be used to learn more:
- [This list of interior mutability in the compiler by nikomatsakis][imlist]

[`rayon`]: https://crates.io/crates/rayon
[Arc]: https://doc.rust-lang.org/std/sync/struct.Arc.html
[imlist]: https://github.com/nikomatsakis/rustc-parallelization/blob/master/interior-mutability-list.md
[irlo0]: https://internals.rust-lang.org/t/parallelizing-rustc-using-rayon/6606
[irlo1]: https://internals.rust-lang.org/t/help-test-parallel-rustc/11503
[monomorphization]: backend/monomorph.md
[parallel-rustdoc]: https://github.com/rust-lang/rust/issues/82741
[Rc]: https://doc.rust-lang.org/std/rc/struct.Rc.html
[rustc-rayon]: https://github.com/rust-lang/rustc-rayon
[tracking]: https://github.com/rust-lang/rust/issues/48685
Loading