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

Erase query result types #107937

Closed
wants to merge 7 commits into from
Closed
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
6 changes: 6 additions & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3983,6 +3983,10 @@ dependencies = [
"winapi",
]

[[package]]
name = "rustc_erase"
version = "0.0.0"

[[package]]
name = "rustc_error_codes"
version = "0.0.0"
Expand Down Expand Up @@ -4373,6 +4377,7 @@ dependencies = [
"rustc_ast",
"rustc_attr",
"rustc_data_structures",
"rustc_erase",
"rustc_error_messages",
"rustc_errors",
"rustc_feature",
Expand Down Expand Up @@ -4571,6 +4576,7 @@ dependencies = [
"rustc-rayon-core",
"rustc_ast",
"rustc_data_structures",
"rustc_erase",
"rustc_errors",
"rustc_hir",
"rustc_index",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_data_structures/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ pub mod stable_hasher;
mod atomic_ref;
pub mod fingerprint;
pub mod profiling;
pub mod remap;
pub mod sharded;
pub mod stack;
pub mod sync;
Expand Down
28 changes: 28 additions & 0 deletions compiler/rustc_data_structures/src/remap.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/// Remaps the type with a different lifetime for 'tcx if applicable.
pub trait Remap {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is rustc_middle::ty::parametrized::ParameterizedOverTcx that does the same thing.

type Remap<'a>;
}

impl Remap for u32 {
type Remap<'a> = u32;
}

impl<T: Remap> Remap for Option<T> {
type Remap<'a> = Option<T::Remap<'a>>;
}

impl Remap for () {
type Remap<'a> = ();
}

impl<T0: Remap, T1: Remap> Remap for (T0, T1) {
type Remap<'a> = (T0::Remap<'a>, T1::Remap<'a>);
}

impl<T0: Remap, T1: Remap, T2: Remap> Remap for (T0, T1, T2) {
type Remap<'a> = (T0::Remap<'a>, T1::Remap<'a>, T2::Remap<'a>);
}

impl<T0: Remap, T1: Remap, T2: Remap, T3: Remap> Remap for (T0, T1, T2, T3) {
type Remap<'a> = (T0::Remap<'a>, T1::Remap<'a>, T2::Remap<'a>, T3::Remap<'a>);
}
6 changes: 1 addition & 5 deletions compiler/rustc_data_structures/src/sharded.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
use crate::fx::{FxHashMap, FxHasher};
use crate::sync::{Lock, LockGuard};
use crate::sync::{CacheAligned, Lock, LockGuard};
use std::borrow::Borrow;
use std::collections::hash_map::RawEntryMut;
use std::hash::{Hash, Hasher};
use std::mem;

#[derive(Clone, Default)]
#[cfg_attr(parallel_compiler, repr(align(64)))]
struct CacheAligned<T>(T);

#[cfg(parallel_compiler)]
// 32 shards is sufficient to reduce contention on an 8-core Ryzen 7 1700,
// but this should be tested on higher core count CPUs. How the `Sharded` type gets used
Expand Down
36 changes: 7 additions & 29 deletions compiler/rustc_data_structures/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ use std::hash::{BuildHasher, Hash};
use std::ops::{Deref, DerefMut};
use std::panic::{catch_unwind, resume_unwind, AssertUnwindSafe};

mod worker_local;
pub use worker_local::{Registry, WorkerLocal};

pub use std::sync::atomic::Ordering;
pub use std::sync::atomic::Ordering::SeqCst;

Expand Down Expand Up @@ -178,33 +181,6 @@ cfg_if! {

use std::cell::Cell;

#[derive(Debug)]
pub struct WorkerLocal<T>(OneThread<T>);

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 thread pool.
#[inline]
pub fn new<F: FnMut(usize) -> T>(mut f: F) -> WorkerLocal<T> {
WorkerLocal(OneThread::new(f(0)))
}

/// Returns the worker-local value for each thread
#[inline]
pub fn into_inner(self) -> Vec<T> {
vec![OneThread::into_inner(self.0)]
}
}

impl<T> Deref for WorkerLocal<T> {
type Target = T;

#[inline(always)]
fn deref(&self) -> &T {
&self.0
}
}

pub type MTRef<'a, T> = &'a mut T;

#[derive(Debug, Default)]
Expand Down Expand Up @@ -324,8 +300,6 @@ cfg_if! {
};
}

pub use rayon_core::WorkerLocal;

pub use rayon::iter::ParallelIterator;
use rayon::iter::IntoParallelIterator;

Expand Down Expand Up @@ -365,6 +339,10 @@ pub fn assert_send<T: ?Sized + Send>() {}
pub fn assert_send_val<T: ?Sized + Send>(_t: &T) {}
pub fn assert_send_sync_val<T: ?Sized + Sync + Send>(_t: &T) {}

#[derive(Clone, Default)]
#[cfg_attr(parallel_compiler, repr(align(64)))]
pub struct CacheAligned<T>(pub T);

pub trait HashMapExt<K, V> {
/// Same as HashMap::insert, but it may panic if there's already an
/// entry for `key` with a value not equal to `value`
Expand Down
189 changes: 189 additions & 0 deletions compiler/rustc_data_structures/src/sync/worker_local.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
use crate::sync::Lock;
use std::cell::Cell;
use std::cell::OnceCell;
use std::ops::Deref;
use std::sync::Arc;

#[cfg(parallel_compiler)]
use {crate::cold_path, crate::sync::CacheAligned};

/// A pointer to the `RegistryData` which uniquely identifies a registry.
/// This identifier can be reused if the registry gets freed.
#[derive(Clone, Copy, Eq, PartialEq)]
struct RegistryId(usize);

impl RegistryId {
#[inline(always)]
/// Verifies that the current thread is associated with the registry and returns its unique
/// index within the registry. This panics if the current thread is not associated with this
/// registry.
///
/// Note that there's a race possible where the identifer in `THREAD_DATA` could be reused
/// so this can succeed from a different registry.
#[cfg(parallel_compiler)]
fn verify(self) -> usize {
let (id, index) = THREAD_DATA.with(|data| (data.registry_id.get(), data.index.get()));

if id == self {
index
} else {
cold_path(|| panic!("Unable to verify registry association"))
}
}
}

struct RegistryData {
thread_limit: usize,
threads: Lock<usize>,
}

/// Represents a list of threads which can access worker locals.
#[derive(Clone)]
pub struct Registry(Arc<RegistryData>);

thread_local! {
/// The registry associated with the thread.
/// This allows the `WorkerLocal` type to clone the registry in its constructor.
static REGISTRY: OnceCell<Registry> = OnceCell::new();
}

struct ThreadData {
registry_id: Cell<RegistryId>,
index: Cell<usize>,
}

thread_local! {
/// A thread local which contains the identifer of `REGISTRY` but allows for faster access.
/// It also holds the index of the current thread.
static THREAD_DATA: ThreadData = const { ThreadData {
registry_id: Cell::new(RegistryId(0)),
index: Cell::new(0),
}};
}

impl Registry {
/// Creates a registry which can hold up to `thread_limit` threads.
pub fn new(thread_limit: usize) -> Self {
Registry(Arc::new(RegistryData { thread_limit, threads: Lock::new(0) }))
}

/// Gets the registry associated with the current thread. Panics if there's no such registry.
pub fn current() -> Self {
REGISTRY.with(|registry| registry.get().cloned().expect("No assocated registry"))
}

/// Registers the current thread with the registry so worker locals can be used on it.
/// Panics if the thread limit is hit or if the thread already has an associated registry.
pub fn register(&self) {
let mut threads = self.0.threads.lock();
if *threads < self.0.thread_limit {
REGISTRY.with(|registry| {
if registry.get().is_some() {
drop(threads);
panic!("Thread already has a registry");
}
registry.set(self.clone()).ok();
THREAD_DATA.with(|data| {
data.registry_id.set(self.id());
data.index.set(*threads);
});
*threads += 1;
});
} else {
drop(threads);
panic!("Thread limit reached");
}
}

/// Gets the identifer of this registry.
fn id(&self) -> RegistryId {
RegistryId(&*self.0 as *const RegistryData as usize)
}
}

/// Holds worker local values for each possible thread in a registry. You can only access the
/// worker local value through the `Deref` impl on the registry associated with the thread it was
/// created on. It will panic otherwise.
pub struct WorkerLocal<T> {
#[cfg(not(parallel_compiler))]
local: T,
#[cfg(parallel_compiler)]
locals: Box<[CacheAligned<T>]>,
#[cfg(parallel_compiler)]
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 the id
// can be reused, but `WorkerLocal` has a reference to `Registry` which will prevent any reuse.
#[cfg(parallel_compiler)]
unsafe impl<T: Send> Sync for WorkerLocal<T> {}

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.
#[inline]
pub fn new<F: FnMut(usize) -> T>(mut initial: F) -> WorkerLocal<T> {
#[cfg(parallel_compiler)]
{
let registry = Registry::current();
WorkerLocal {
locals: {
let locals: Vec<_> =
(0..registry.0.thread_limit).map(|i| CacheAligned(initial(i))).collect();
locals.into_boxed_slice()
},
registry,
}
}
#[cfg(not(parallel_compiler))]
{
WorkerLocal { local: initial(0) }
}
}

/// Returns the worker-local value for each thread
#[inline]
pub fn into_inner(self) -> Vec<T> {
#[cfg(parallel_compiler)]
{
self.locals.into_vec().into_iter().map(|local| local.0).collect()
}
#[cfg(not(parallel_compiler))]
{
vec![self.local]
}
}
}

impl<T> WorkerLocal<Vec<T>> {
/// Joins the elements of all the worker locals into one Vec
pub fn join(self) -> Vec<T> {
self.into_inner().into_iter().flat_map(|v| v).collect()
}
}

impl<T: Default> Default for WorkerLocal<T> {
fn default() -> Self {
WorkerLocal::new(|_| T::default())
}
}

impl<T> Deref for WorkerLocal<T> {
type Target = T;

#[inline(always)]
#[cfg(not(parallel_compiler))]
fn deref(&self) -> &T {
&self.local
}

#[inline(always)]
#[cfg(parallel_compiler)]
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.
unsafe { &self.locals.get_unchecked(self.registry.id().verify()).0 }
}
}
6 changes: 6 additions & 0 deletions compiler/rustc_erase/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
name = "rustc_erase"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this inside a new crate and not just in rustc_data_structures? Creating a crate for 47 lines seems pointless, even if the code is very unsafe and self-contained

Copy link
Member

Choose a reason for hiding this comment

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

// This is a separate crate so that we can `allow(incomplete_features)` for just `generic_const_exprs`

Copy link
Member

Choose a reason for hiding this comment

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

Ah, didn't see that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that generic_const_exprs will cause ICEs in other crates than the one using the feature if any of it is used in public APIs

version = "0.0.0"
edition = "2021"

[lib]
47 changes: 47 additions & 0 deletions compiler/rustc_erase/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// This is a separate crate so that we can `allow(incomplete_features)` for just `generic_const_exprs`
#![feature(generic_const_exprs)]
Copy link
Member

@BoxyUwU BoxyUwU Feb 12, 2023

Choose a reason for hiding this comment

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

Please do not use generic_const_exprs, its incredibly incomplete/broken and also its unsound and will have changes made to it that breaks code that works currently in order to get closer to an actually functioning feature. I do not want to have to deal with cfg(bootstrap) pain and working around generic_const_expr bugs from usage of the feature inside of rustc whenever this feature gets worked on ^^'

Copy link
Member

Choose a reason for hiding this comment

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

I guess to explicitly answer

for an option on how stable feature(generic_const_exprs) is.

extremely very not stable 😅

#![allow(incomplete_features)]
#![feature(core_intrinsics)]

#[cfg(debug_assertions)]
use std::intrinsics::type_name;
use std::{
fmt,
mem::{size_of, transmute_copy, MaybeUninit},
};

#[derive(Copy, Clone)]
pub struct Erased<const N: usize> {
data: MaybeUninit<[u8; N]>,
#[cfg(debug_assertions)]
type_id: &'static str,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we prefer std::any::TypeId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That requires 'static.

}

impl<const N: usize> fmt::Debug for Erased<N> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "Erased<{}>", N)
}
}

pub type Erase<T> = Erased<{ size_of::<T>() }>;

#[inline(always)]
pub fn erase<T: Copy>(src: T) -> Erased<{ size_of::<T>() }> {
Erased {
// SAFETY:: Is it safe to transmute to MaybeUninit
data: unsafe { transmute_copy(&src) },
#[cfg(debug_assertions)]
type_id: type_name::<T>(),
}
}

/// Restores an erased value.
///
/// This is only safe if `value` is a valid instance of `T`.
/// For example if `T` was erased with `erase` previously.
#[inline(always)]
pub unsafe fn restore<T: Copy>(value: Erased<{ size_of::<T>() }>) -> T {
#[cfg(debug_assertions)]
assert_eq!(value.type_id, type_name::<T>());
unsafe { transmute_copy(&value.data) }
}
Loading