From 0c96a9260b75b97e5ce87d77b68d222ae4a3d988 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Thu, 31 Aug 2023 09:16:33 +0200 Subject: [PATCH 1/7] Add `Freeze` type and use it to store `Definitions` --- compiler/rustc_data_structures/src/sync.rs | 3 + .../rustc_data_structures/src/sync/freeze.rs | 104 ++++++++++++++++++ compiler/rustc_interface/src/queries.rs | 4 +- compiler/rustc_middle/src/hir/map/mod.rs | 2 +- compiler/rustc_middle/src/ty/context.rs | 20 ++-- compiler/rustc_session/src/cstore.rs | 4 +- 6 files changed, 122 insertions(+), 15 deletions(-) create mode 100644 compiler/rustc_data_structures/src/sync/freeze.rs diff --git a/compiler/rustc_data_structures/src/sync.rs b/compiler/rustc_data_structures/src/sync.rs index e82b0f6d49673..a8f42e0ed0e25 100644 --- a/compiler/rustc_data_structures/src/sync.rs +++ b/compiler/rustc_data_structures/src/sync.rs @@ -61,6 +61,9 @@ pub use vec::{AppendOnlyIndexVec, AppendOnlyVec}; mod vec; +mod freeze; +pub use freeze::{Freeze, FreezeReadGuard, FreezeWriteGuard}; + mod mode { use super::Ordering; use std::sync::atomic::AtomicU8; diff --git a/compiler/rustc_data_structures/src/sync/freeze.rs b/compiler/rustc_data_structures/src/sync/freeze.rs new file mode 100644 index 0000000000000..77b37af526940 --- /dev/null +++ b/compiler/rustc_data_structures/src/sync/freeze.rs @@ -0,0 +1,104 @@ +use crate::sync::{AtomicBool, Lock, LockGuard}; +#[cfg(parallel_compiler)] +use crate::sync::{DynSend, DynSync}; +use std::{ + cell::UnsafeCell, + ops::{Deref, DerefMut}, + sync::atomic::Ordering, +}; + +/// A type which allows mutation using a lock until +/// the value is frozen and can be accessed lock-free. +#[derive(Default)] +pub struct Freeze { + data: UnsafeCell, + frozen: AtomicBool, + lock: Lock<()>, +} + +#[cfg(parallel_compiler)] +unsafe impl DynSync for Freeze {} + +impl Freeze { + #[inline] + pub fn new(value: T) -> Self { + Self { data: UnsafeCell::new(value), frozen: AtomicBool::new(false), lock: Lock::new(()) } + } + + #[inline] + pub fn read(&self) -> FreezeReadGuard<'_, T> { + FreezeReadGuard { + _lock_guard: if self.frozen.load(Ordering::Acquire) { + None + } else { + Some(self.lock.lock()) + }, + // SAFETY: If this is not frozen, `_lock_guard` holds the lock to the `UnsafeCell` so + // this has shared access until the `FreezeReadGuard` is dropped. If this is frozen, + // the data cannot be modified and shared access is sound. + data: unsafe { &*self.data.get() }, + } + } + + #[inline] + #[track_caller] + pub fn write(&self) -> FreezeWriteGuard<'_, T> { + let _lock_guard = self.lock.lock(); + assert!(!self.frozen.load(Ordering::Relaxed), "still mutable"); + FreezeWriteGuard { + _lock_guard, + // SAFETY: `_lock_guard` holds the lock to the `UnsafeCell` so this has mutable access + // until the `FreezeWriteGuard` is dropped. + data: unsafe { &mut *self.data.get() }, + } + } + + #[inline] + pub fn freeze(&self) -> &T { + if !self.frozen.load(Ordering::Acquire) { + // Get the lock to ensure no concurrent writes. + let _lock = self.lock.lock(); + self.frozen.store(true, Ordering::Release); + } + + // SAFETY: This is frozen so the data cannot be modified and shared access is sound. + unsafe { &*self.data.get() } + } +} + +/// A guard holding shared access to a `Freeze` which is in a locked state or frozen. +#[must_use = "if unused the Freeze may immediately unlock"] +pub struct FreezeReadGuard<'a, T> { + _lock_guard: Option>, + data: &'a T, +} + +impl<'a, T: 'a> Deref for FreezeReadGuard<'a, T> { + type Target = T; + #[inline] + fn deref(&self) -> &T { + self.data + } +} + +/// A guard holding mutable access to a `Freeze` which is in a locked state or frozen. +#[must_use = "if unused the Freeze may immediately unlock"] +pub struct FreezeWriteGuard<'a, T> { + _lock_guard: LockGuard<'a, ()>, + data: &'a mut T, +} + +impl<'a, T: 'a> Deref for FreezeWriteGuard<'a, T> { + type Target = T; + #[inline] + fn deref(&self) -> &T { + self.data + } +} + +impl<'a, T: 'a> DerefMut for FreezeWriteGuard<'a, T> { + #[inline] + fn deref_mut(&mut self) -> &mut T { + self.data + } +} diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs index 2db7aa0e36710..a7c6368074713 100644 --- a/compiler/rustc_interface/src/queries.rs +++ b/compiler/rustc_interface/src/queries.rs @@ -7,7 +7,7 @@ use rustc_codegen_ssa::traits::CodegenBackend; use rustc_codegen_ssa::CodegenResults; use rustc_data_structures::steal::Steal; use rustc_data_structures::svh::Svh; -use rustc_data_structures::sync::{AppendOnlyIndexVec, Lrc, OnceLock, RwLock, WorkerLocal}; +use rustc_data_structures::sync::{AppendOnlyIndexVec, Freeze, Lrc, OnceLock, RwLock, WorkerLocal}; use rustc_hir::def_id::{StableCrateId, CRATE_DEF_ID, LOCAL_CRATE}; use rustc_hir::definitions::Definitions; use rustc_incremental::DepGraphFuture; @@ -197,7 +197,7 @@ impl<'tcx> Queries<'tcx> { self.codegen_backend().metadata_loader(), stable_crate_id, )) as _); - let definitions = RwLock::new(Definitions::new(stable_crate_id)); + let definitions = Freeze::new(Definitions::new(stable_crate_id)); let source_span = AppendOnlyIndexVec::new(); let _id = source_span.push(krate.spans.inner_span); debug_assert_eq!(_id, CRATE_DEF_ID); diff --git a/compiler/rustc_middle/src/hir/map/mod.rs b/compiler/rustc_middle/src/hir/map/mod.rs index 3e8f07ed233a2..e20a202561ff0 100644 --- a/compiler/rustc_middle/src/hir/map/mod.rs +++ b/compiler/rustc_middle/src/hir/map/mod.rs @@ -1207,7 +1207,7 @@ pub(super) fn crate_hash(tcx: TyCtxt<'_>, _: LocalCrate) -> Svh { source_file_names.hash_stable(&mut hcx, &mut stable_hasher); debugger_visualizers.hash_stable(&mut hcx, &mut stable_hasher); if tcx.sess.opts.incremental_relative_spans() { - let definitions = tcx.definitions_untracked(); + let definitions = tcx.untracked().definitions.freeze(); let mut owner_spans: Vec<_> = krate .owners .iter_enumerated() diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index e2f9e299566ae..90d847804f0e5 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -39,7 +39,9 @@ use rustc_data_structures::profiling::SelfProfilerRef; use rustc_data_structures::sharded::{IntoPointer, ShardedHashMap}; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_data_structures::steal::Steal; -use rustc_data_structures::sync::{self, Lock, Lrc, MappedReadGuard, ReadGuard, WorkerLocal}; +use rustc_data_structures::sync::{ + self, FreezeReadGuard, Lock, Lrc, MappedReadGuard, ReadGuard, WorkerLocal, +}; use rustc_data_structures::unord::UnordSet; use rustc_errors::{ DecorateLint, DiagnosticBuilder, DiagnosticMessage, ErrorGuaranteed, MultiSpan, @@ -964,8 +966,8 @@ impl<'tcx> TyCtxt<'tcx> { i += 1; } - // Leak a read lock once we finish iterating on definitions, to prevent adding new ones. - definitions.leak(); + // Freeze definitions once we finish iterating on them, to prevent adding new ones. + definitions.freeze(); }) } @@ -974,10 +976,9 @@ impl<'tcx> TyCtxt<'tcx> { // definitions change. self.dep_graph.read_index(DepNodeIndex::FOREVER_RED_NODE); - // Leak a read lock once we start iterating on definitions, to prevent adding new ones + // Freeze definitions once we start iterating on them, to prevent adding new ones // while iterating. If some query needs to add definitions, it should be `ensure`d above. - let definitions = self.untracked.definitions.leak(); - definitions.def_path_table() + self.untracked.definitions.freeze().def_path_table() } pub fn def_path_hash_to_def_index_map( @@ -986,10 +987,9 @@ impl<'tcx> TyCtxt<'tcx> { // Create a dependency to the crate to be sure we re-execute this when the amount of // definitions change. self.ensure().hir_crate(()); - // Leak a read lock once we start iterating on definitions, to prevent adding new ones + // Freeze definitions once we start iterating on them, to prevent adding new ones // while iterating. If some query needs to add definitions, it should be `ensure`d above. - let definitions = self.untracked.definitions.leak(); - definitions.def_path_hash_to_def_index_map() + self.untracked.definitions.freeze().def_path_hash_to_def_index_map() } /// Note that this is *untracked* and should only be used within the query @@ -1006,7 +1006,7 @@ impl<'tcx> TyCtxt<'tcx> { /// Note that this is *untracked* and should only be used within the query /// system if the result is otherwise tracked through queries #[inline] - pub fn definitions_untracked(self) -> ReadGuard<'tcx, Definitions> { + pub fn definitions_untracked(self) -> FreezeReadGuard<'tcx, Definitions> { self.untracked.definitions.read() } diff --git a/compiler/rustc_session/src/cstore.rs b/compiler/rustc_session/src/cstore.rs index c53a355b533ea..a86363b9cae9b 100644 --- a/compiler/rustc_session/src/cstore.rs +++ b/compiler/rustc_session/src/cstore.rs @@ -7,7 +7,7 @@ use crate::utils::NativeLibKind; use crate::Session; use rustc_ast as ast; use rustc_data_structures::owned_slice::OwnedSlice; -use rustc_data_structures::sync::{self, AppendOnlyIndexVec, RwLock}; +use rustc_data_structures::sync::{self, AppendOnlyIndexVec, Freeze, RwLock}; use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, StableCrateId, LOCAL_CRATE}; use rustc_hir::definitions::{DefKey, DefPath, DefPathHash, Definitions}; use rustc_span::hygiene::{ExpnHash, ExpnId}; @@ -261,5 +261,5 @@ pub struct Untracked { pub cstore: RwLock>, /// Reference span for definitions. pub source_span: AppendOnlyIndexVec, - pub definitions: RwLock, + pub definitions: Freeze, } From 6881eed8f686964e83d5b915fcc1445f23aa60ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Thu, 31 Aug 2023 23:14:47 +0200 Subject: [PATCH 2/7] Don't hold the definitions' lock across `index_hir` --- compiler/rustc_ast_lowering/src/index.rs | 30 ++++++++++++------------ compiler/rustc_ast_lowering/src/lib.rs | 3 +-- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_ast_lowering/src/index.rs b/compiler/rustc_ast_lowering/src/index.rs index ce847906fb99a..eff362f3ff0cd 100644 --- a/compiler/rustc_ast_lowering/src/index.rs +++ b/compiler/rustc_ast_lowering/src/index.rs @@ -2,19 +2,17 @@ use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::sorted_map::SortedMap; use rustc_hir as hir; use rustc_hir::def_id::LocalDefId; -use rustc_hir::definitions; use rustc_hir::intravisit::{self, Visitor}; use rustc_hir::*; use rustc_index::{Idx, IndexVec}; use rustc_middle::span_bug; -use rustc_session::Session; -use rustc_span::source_map::SourceMap; +use rustc_middle::ty::TyCtxt; use rustc_span::{Span, DUMMY_SP}; /// A visitor that walks over the HIR and collects `Node`s into a HIR map. pub(super) struct NodeCollector<'a, 'hir> { - /// Source map - source_map: &'a SourceMap, + tcx: TyCtxt<'hir>, + bodies: &'a SortedMap>, /// Outputs @@ -25,14 +23,11 @@ pub(super) struct NodeCollector<'a, 'hir> { parent_node: hir::ItemLocalId, owner: OwnerId, - - definitions: &'a definitions::Definitions, } -#[instrument(level = "debug", skip(sess, definitions, bodies))] +#[instrument(level = "debug", skip(tcx, bodies))] pub(super) fn index_hir<'hir>( - sess: &Session, - definitions: &definitions::Definitions, + tcx: TyCtxt<'hir>, item: hir::OwnerNode<'hir>, bodies: &SortedMap>, ) -> (IndexVec>>, FxHashMap) { @@ -42,8 +37,7 @@ pub(super) fn index_hir<'hir>( // used. nodes.push(Some(ParentedNode { parent: ItemLocalId::INVALID, node: item.into() })); let mut collector = NodeCollector { - source_map: sess.source_map(), - definitions, + tcx, owner: item.def_id(), parent_node: ItemLocalId::new(0), nodes, @@ -79,11 +73,17 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> { span, "inconsistent HirId at `{:?}` for `{:?}`: \ current_dep_node_owner={} ({:?}), hir_id.owner={} ({:?})", - self.source_map.span_to_diagnostic_string(span), + self.tcx.sess.source_map().span_to_diagnostic_string(span), node, - self.definitions.def_path(self.owner.def_id).to_string_no_crate_verbose(), + self.tcx + .definitions_untracked() + .def_path(self.owner.def_id) + .to_string_no_crate_verbose(), self.owner, - self.definitions.def_path(hir_id.owner.def_id).to_string_no_crate_verbose(), + self.tcx + .definitions_untracked() + .def_path(hir_id.owner.def_id) + .to_string_no_crate_verbose(), hir_id.owner, ) } diff --git a/compiler/rustc_ast_lowering/src/lib.rs b/compiler/rustc_ast_lowering/src/lib.rs index a6d1ef33f4069..41db61d391a13 100644 --- a/compiler/rustc_ast_lowering/src/lib.rs +++ b/compiler/rustc_ast_lowering/src/lib.rs @@ -671,8 +671,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> { } else { (None, None) }; - let (nodes, parenting) = - index::index_hir(self.tcx.sess, &*self.tcx.definitions_untracked(), node, &bodies); + let (nodes, parenting) = index::index_hir(self.tcx, node, &bodies); let nodes = hir::OwnerNodes { opt_hash_including_bodies, nodes, bodies }; let attrs = hir::AttributeMap { map: attrs, opt_hash: attrs_hash }; From ac54d4957268b684ef36dbda8ab9d17aa09e3333 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 2 Sep 2023 00:25:54 +0200 Subject: [PATCH 3/7] Freeze `Definitions` earlier --- compiler/rustc_hir_analysis/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compiler/rustc_hir_analysis/src/lib.rs b/compiler/rustc_hir_analysis/src/lib.rs index 4f95174f869e7..b0f333b79cafe 100644 --- a/compiler/rustc_hir_analysis/src/lib.rs +++ b/compiler/rustc_hir_analysis/src/lib.rs @@ -237,6 +237,10 @@ pub fn check_crate(tcx: TyCtxt<'_>) -> Result<(), ErrorGuaranteed> { tcx.hir().for_each_module(|module| tcx.ensure().check_mod_item_types(module)) }); + // Freeze definitions as we don't add new ones at this point. This improves performance by + // allowing lock-free access to them. + tcx.untracked().definitions.freeze(); + // FIXME: Remove this when we implement creating `DefId`s // for anon constants during their parents' typeck. // Typeck all body owners in parallel will produce queries From 25884ec9be2115c729ac114fd6c62e4037584b08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 2 Sep 2023 00:46:15 +0200 Subject: [PATCH 4/7] Use `RwLock` for `Freeze` --- .../rustc_data_structures/src/sync/freeze.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_data_structures/src/sync/freeze.rs b/compiler/rustc_data_structures/src/sync/freeze.rs index 77b37af526940..21bbc35e317dd 100644 --- a/compiler/rustc_data_structures/src/sync/freeze.rs +++ b/compiler/rustc_data_structures/src/sync/freeze.rs @@ -1,4 +1,4 @@ -use crate::sync::{AtomicBool, Lock, LockGuard}; +use crate::sync::{AtomicBool, ReadGuard, RwLock, WriteGuard}; #[cfg(parallel_compiler)] use crate::sync::{DynSend, DynSync}; use std::{ @@ -13,7 +13,9 @@ use std::{ pub struct Freeze { data: UnsafeCell, frozen: AtomicBool, - lock: Lock<()>, + + /// This lock protects writes to the `data` and `frozen` fields. + lock: RwLock<()>, } #[cfg(parallel_compiler)] @@ -22,7 +24,7 @@ unsafe impl DynSync for Freeze {} impl Freeze { #[inline] pub fn new(value: T) -> Self { - Self { data: UnsafeCell::new(value), frozen: AtomicBool::new(false), lock: Lock::new(()) } + Self { data: UnsafeCell::new(value), frozen: AtomicBool::new(false), lock: RwLock::new(()) } } #[inline] @@ -31,7 +33,7 @@ impl Freeze { _lock_guard: if self.frozen.load(Ordering::Acquire) { None } else { - Some(self.lock.lock()) + Some(self.lock.read()) }, // SAFETY: If this is not frozen, `_lock_guard` holds the lock to the `UnsafeCell` so // this has shared access until the `FreezeReadGuard` is dropped. If this is frozen, @@ -43,7 +45,7 @@ impl Freeze { #[inline] #[track_caller] pub fn write(&self) -> FreezeWriteGuard<'_, T> { - let _lock_guard = self.lock.lock(); + let _lock_guard = self.lock.write(); assert!(!self.frozen.load(Ordering::Relaxed), "still mutable"); FreezeWriteGuard { _lock_guard, @@ -57,7 +59,7 @@ impl Freeze { pub fn freeze(&self) -> &T { if !self.frozen.load(Ordering::Acquire) { // Get the lock to ensure no concurrent writes. - let _lock = self.lock.lock(); + let _lock = self.lock.write(); self.frozen.store(true, Ordering::Release); } @@ -69,7 +71,7 @@ impl Freeze { /// A guard holding shared access to a `Freeze` which is in a locked state or frozen. #[must_use = "if unused the Freeze may immediately unlock"] pub struct FreezeReadGuard<'a, T> { - _lock_guard: Option>, + _lock_guard: Option>, data: &'a T, } @@ -84,7 +86,7 @@ impl<'a, T: 'a> Deref for FreezeReadGuard<'a, T> { /// A guard holding mutable access to a `Freeze` which is in a locked state or frozen. #[must_use = "if unused the Freeze may immediately unlock"] pub struct FreezeWriteGuard<'a, T> { - _lock_guard: LockGuard<'a, ()>, + _lock_guard: WriteGuard<'a, ()>, data: &'a mut T, } From 50f0d666d3308f0b0a45842cbe725fcb5b3c3861 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 2 Sep 2023 01:22:22 +0200 Subject: [PATCH 5/7] Add some comments --- compiler/rustc_data_structures/src/sync/freeze.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_data_structures/src/sync/freeze.rs b/compiler/rustc_data_structures/src/sync/freeze.rs index 21bbc35e317dd..e9ad6ee8394d0 100644 --- a/compiler/rustc_data_structures/src/sync/freeze.rs +++ b/compiler/rustc_data_structures/src/sync/freeze.rs @@ -9,6 +9,8 @@ use std::{ /// A type which allows mutation using a lock until /// the value is frozen and can be accessed lock-free. +/// +/// Unlike `RwLock`, it can be used to prevent mutation past a point. #[derive(Default)] pub struct Freeze { data: UnsafeCell, @@ -46,6 +48,7 @@ impl Freeze { #[track_caller] pub fn write(&self) -> FreezeWriteGuard<'_, T> { let _lock_guard = self.lock.write(); + // Use relaxed ordering since we're in the write lock. assert!(!self.frozen.load(Ordering::Relaxed), "still mutable"); FreezeWriteGuard { _lock_guard, @@ -58,7 +61,7 @@ impl Freeze { #[inline] pub fn freeze(&self) -> &T { if !self.frozen.load(Ordering::Acquire) { - // Get the lock to ensure no concurrent writes. + // Get the lock to ensure no concurrent writes and that we release the latest write. let _lock = self.lock.write(); self.frozen.store(true, Ordering::Release); } From a3ad045ea49b0613505e02bb0036575d1fc0c99f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 2 Sep 2023 01:28:04 +0200 Subject: [PATCH 6/7] Rename `Freeze` to `FreezeLock` --- compiler/rustc_data_structures/src/sync.rs | 2 +- compiler/rustc_data_structures/src/sync/freeze.rs | 14 +++++++------- compiler/rustc_interface/src/queries.rs | 6 ++++-- compiler/rustc_session/src/cstore.rs | 4 ++-- 4 files changed, 14 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_data_structures/src/sync.rs b/compiler/rustc_data_structures/src/sync.rs index a8f42e0ed0e25..8eda9043e358b 100644 --- a/compiler/rustc_data_structures/src/sync.rs +++ b/compiler/rustc_data_structures/src/sync.rs @@ -62,7 +62,7 @@ pub use vec::{AppendOnlyIndexVec, AppendOnlyVec}; mod vec; mod freeze; -pub use freeze::{Freeze, FreezeReadGuard, FreezeWriteGuard}; +pub use freeze::{FreezeLock, FreezeReadGuard, FreezeWriteGuard}; mod mode { use super::Ordering; diff --git a/compiler/rustc_data_structures/src/sync/freeze.rs b/compiler/rustc_data_structures/src/sync/freeze.rs index e9ad6ee8394d0..ea75dcc4151da 100644 --- a/compiler/rustc_data_structures/src/sync/freeze.rs +++ b/compiler/rustc_data_structures/src/sync/freeze.rs @@ -12,7 +12,7 @@ use std::{ /// /// Unlike `RwLock`, it can be used to prevent mutation past a point. #[derive(Default)] -pub struct Freeze { +pub struct FreezeLock { data: UnsafeCell, frozen: AtomicBool, @@ -21,9 +21,9 @@ pub struct Freeze { } #[cfg(parallel_compiler)] -unsafe impl DynSync for Freeze {} +unsafe impl DynSync for FreezeLock {} -impl Freeze { +impl FreezeLock { #[inline] pub fn new(value: T) -> Self { Self { data: UnsafeCell::new(value), frozen: AtomicBool::new(false), lock: RwLock::new(()) } @@ -71,8 +71,8 @@ impl Freeze { } } -/// A guard holding shared access to a `Freeze` which is in a locked state or frozen. -#[must_use = "if unused the Freeze may immediately unlock"] +/// A guard holding shared access to a `FreezeLock` which is in a locked state or frozen. +#[must_use = "if unused the FreezeLock may immediately unlock"] pub struct FreezeReadGuard<'a, T> { _lock_guard: Option>, data: &'a T, @@ -86,8 +86,8 @@ impl<'a, T: 'a> Deref for FreezeReadGuard<'a, T> { } } -/// A guard holding mutable access to a `Freeze` which is in a locked state or frozen. -#[must_use = "if unused the Freeze may immediately unlock"] +/// A guard holding mutable access to a `FreezeLock` which is in a locked state or frozen. +#[must_use = "if unused the FreezeLock may immediately unlock"] pub struct FreezeWriteGuard<'a, T> { _lock_guard: WriteGuard<'a, ()>, data: &'a mut T, diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs index a7c6368074713..e0d9998d919b1 100644 --- a/compiler/rustc_interface/src/queries.rs +++ b/compiler/rustc_interface/src/queries.rs @@ -7,7 +7,9 @@ use rustc_codegen_ssa::traits::CodegenBackend; use rustc_codegen_ssa::CodegenResults; use rustc_data_structures::steal::Steal; use rustc_data_structures::svh::Svh; -use rustc_data_structures::sync::{AppendOnlyIndexVec, Freeze, Lrc, OnceLock, RwLock, WorkerLocal}; +use rustc_data_structures::sync::{ + AppendOnlyIndexVec, FreezeLock, Lrc, OnceLock, RwLock, WorkerLocal, +}; use rustc_hir::def_id::{StableCrateId, CRATE_DEF_ID, LOCAL_CRATE}; use rustc_hir::definitions::Definitions; use rustc_incremental::DepGraphFuture; @@ -197,7 +199,7 @@ impl<'tcx> Queries<'tcx> { self.codegen_backend().metadata_loader(), stable_crate_id, )) as _); - let definitions = Freeze::new(Definitions::new(stable_crate_id)); + let definitions = FreezeLock::new(Definitions::new(stable_crate_id)); let source_span = AppendOnlyIndexVec::new(); let _id = source_span.push(krate.spans.inner_span); debug_assert_eq!(_id, CRATE_DEF_ID); diff --git a/compiler/rustc_session/src/cstore.rs b/compiler/rustc_session/src/cstore.rs index a86363b9cae9b..90f57be9324ab 100644 --- a/compiler/rustc_session/src/cstore.rs +++ b/compiler/rustc_session/src/cstore.rs @@ -7,7 +7,7 @@ use crate::utils::NativeLibKind; use crate::Session; use rustc_ast as ast; use rustc_data_structures::owned_slice::OwnedSlice; -use rustc_data_structures::sync::{self, AppendOnlyIndexVec, Freeze, RwLock}; +use rustc_data_structures::sync::{self, AppendOnlyIndexVec, FreezeLock, RwLock}; use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, StableCrateId, LOCAL_CRATE}; use rustc_hir::definitions::{DefKey, DefPath, DefPathHash, Definitions}; use rustc_span::hygiene::{ExpnHash, ExpnId}; @@ -261,5 +261,5 @@ pub struct Untracked { pub cstore: RwLock>, /// Reference span for definitions. pub source_span: AppendOnlyIndexVec, - pub definitions: Freeze, + pub definitions: FreezeLock, } From 35e8b67fc2a48dc77245e805f3976acd51d45474 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 2 Sep 2023 08:21:07 +0200 Subject: [PATCH 7/7] Use a reference to the lock in the guards --- .../rustc_data_structures/src/sync/freeze.rs | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_data_structures/src/sync/freeze.rs b/compiler/rustc_data_structures/src/sync/freeze.rs index ea75dcc4151da..d9f1d72d8510a 100644 --- a/compiler/rustc_data_structures/src/sync/freeze.rs +++ b/compiler/rustc_data_structures/src/sync/freeze.rs @@ -3,6 +3,7 @@ use crate::sync::{AtomicBool, ReadGuard, RwLock, WriteGuard}; use crate::sync::{DynSend, DynSync}; use std::{ cell::UnsafeCell, + marker::PhantomData, ops::{Deref, DerefMut}, sync::atomic::Ordering, }; @@ -37,10 +38,7 @@ impl FreezeLock { } else { Some(self.lock.read()) }, - // SAFETY: If this is not frozen, `_lock_guard` holds the lock to the `UnsafeCell` so - // this has shared access until the `FreezeReadGuard` is dropped. If this is frozen, - // the data cannot be modified and shared access is sound. - data: unsafe { &*self.data.get() }, + lock: self, } } @@ -50,12 +48,7 @@ impl FreezeLock { let _lock_guard = self.lock.write(); // Use relaxed ordering since we're in the write lock. assert!(!self.frozen.load(Ordering::Relaxed), "still mutable"); - FreezeWriteGuard { - _lock_guard, - // SAFETY: `_lock_guard` holds the lock to the `UnsafeCell` so this has mutable access - // until the `FreezeWriteGuard` is dropped. - data: unsafe { &mut *self.data.get() }, - } + FreezeWriteGuard { _lock_guard, lock: self, marker: PhantomData } } #[inline] @@ -75,14 +68,17 @@ impl FreezeLock { #[must_use = "if unused the FreezeLock may immediately unlock"] pub struct FreezeReadGuard<'a, T> { _lock_guard: Option>, - data: &'a T, + lock: &'a FreezeLock, } impl<'a, T: 'a> Deref for FreezeReadGuard<'a, T> { type Target = T; #[inline] fn deref(&self) -> &T { - self.data + // SAFETY: If `lock` is not frozen, `_lock_guard` holds the lock to the `UnsafeCell` so + // this has shared access until the `FreezeReadGuard` is dropped. If `lock` is frozen, + // the data cannot be modified and shared access is sound. + unsafe { &*self.lock.data.get() } } } @@ -90,20 +86,23 @@ impl<'a, T: 'a> Deref for FreezeReadGuard<'a, T> { #[must_use = "if unused the FreezeLock may immediately unlock"] pub struct FreezeWriteGuard<'a, T> { _lock_guard: WriteGuard<'a, ()>, - data: &'a mut T, + lock: &'a FreezeLock, + marker: PhantomData<&'a mut T>, } impl<'a, T: 'a> Deref for FreezeWriteGuard<'a, T> { type Target = T; #[inline] fn deref(&self) -> &T { - self.data + // SAFETY: `self._lock_guard` holds the lock to the `UnsafeCell` so this has shared access. + unsafe { &*self.lock.data.get() } } } impl<'a, T: 'a> DerefMut for FreezeWriteGuard<'a, T> { #[inline] fn deref_mut(&mut self) -> &mut T { - self.data + // SAFETY: `self._lock_guard` holds the lock to the `UnsafeCell` so this has mutable access. + unsafe { &mut *self.lock.data.get() } } }