Skip to content

Commit a0c28cd

Browse files
committed
Auto merge of #115401 - Zoxc:freeze, r=oli-obk
Add `FreezeLock` type and use it to store `Definitions` This adds a `FreezeLock` type which allows mutation using a lock until the value is frozen where it can be accessed lock-free. It's used to store `Definitions` in `Untracked` instead of a `RwLock`. Unlike the current scheme of leaking read guards this doesn't deadlock if definitions is written to after no mutation are expected.
2 parents c1d80ba + 35e8b67 commit a0c28cd

File tree

9 files changed

+148
-32
lines changed

9 files changed

+148
-32
lines changed

compiler/rustc_ast_lowering/src/index.rs

+15-15
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,17 @@ use rustc_data_structures::fx::FxHashMap;
22
use rustc_data_structures::sorted_map::SortedMap;
33
use rustc_hir as hir;
44
use rustc_hir::def_id::LocalDefId;
5-
use rustc_hir::definitions;
65
use rustc_hir::intravisit::{self, Visitor};
76
use rustc_hir::*;
87
use rustc_index::{Idx, IndexVec};
98
use rustc_middle::span_bug;
10-
use rustc_session::Session;
11-
use rustc_span::source_map::SourceMap;
9+
use rustc_middle::ty::TyCtxt;
1210
use rustc_span::{Span, DUMMY_SP};
1311

1412
/// A visitor that walks over the HIR and collects `Node`s into a HIR map.
1513
pub(super) struct NodeCollector<'a, 'hir> {
16-
/// Source map
17-
source_map: &'a SourceMap,
14+
tcx: TyCtxt<'hir>,
15+
1816
bodies: &'a SortedMap<ItemLocalId, &'hir Body<'hir>>,
1917

2018
/// Outputs
@@ -25,14 +23,11 @@ pub(super) struct NodeCollector<'a, 'hir> {
2523
parent_node: hir::ItemLocalId,
2624

2725
owner: OwnerId,
28-
29-
definitions: &'a definitions::Definitions,
3026
}
3127

32-
#[instrument(level = "debug", skip(sess, definitions, bodies))]
28+
#[instrument(level = "debug", skip(tcx, bodies))]
3329
pub(super) fn index_hir<'hir>(
34-
sess: &Session,
35-
definitions: &definitions::Definitions,
30+
tcx: TyCtxt<'hir>,
3631
item: hir::OwnerNode<'hir>,
3732
bodies: &SortedMap<ItemLocalId, &'hir Body<'hir>>,
3833
) -> (IndexVec<ItemLocalId, Option<ParentedNode<'hir>>>, FxHashMap<LocalDefId, ItemLocalId>) {
@@ -42,8 +37,7 @@ pub(super) fn index_hir<'hir>(
4237
// used.
4338
nodes.push(Some(ParentedNode { parent: ItemLocalId::INVALID, node: item.into() }));
4439
let mut collector = NodeCollector {
45-
source_map: sess.source_map(),
46-
definitions,
40+
tcx,
4741
owner: item.def_id(),
4842
parent_node: ItemLocalId::new(0),
4943
nodes,
@@ -79,11 +73,17 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
7973
span,
8074
"inconsistent HirId at `{:?}` for `{:?}`: \
8175
current_dep_node_owner={} ({:?}), hir_id.owner={} ({:?})",
82-
self.source_map.span_to_diagnostic_string(span),
76+
self.tcx.sess.source_map().span_to_diagnostic_string(span),
8377
node,
84-
self.definitions.def_path(self.owner.def_id).to_string_no_crate_verbose(),
78+
self.tcx
79+
.definitions_untracked()
80+
.def_path(self.owner.def_id)
81+
.to_string_no_crate_verbose(),
8582
self.owner,
86-
self.definitions.def_path(hir_id.owner.def_id).to_string_no_crate_verbose(),
83+
self.tcx
84+
.definitions_untracked()
85+
.def_path(hir_id.owner.def_id)
86+
.to_string_no_crate_verbose(),
8787
hir_id.owner,
8888
)
8989
}

compiler/rustc_ast_lowering/src/lib.rs

+1-2
Original file line numberDiff line numberDiff line change
@@ -671,8 +671,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
671671
} else {
672672
(None, None)
673673
};
674-
let (nodes, parenting) =
675-
index::index_hir(self.tcx.sess, &*self.tcx.definitions_untracked(), node, &bodies);
674+
let (nodes, parenting) = index::index_hir(self.tcx, node, &bodies);
676675
let nodes = hir::OwnerNodes { opt_hash_including_bodies, nodes, bodies };
677676
let attrs = hir::AttributeMap { map: attrs, opt_hash: attrs_hash };
678677

compiler/rustc_data_structures/src/sync.rs

+3
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,9 @@ pub use vec::{AppendOnlyIndexVec, AppendOnlyVec};
6161

6262
mod vec;
6363

64+
mod freeze;
65+
pub use freeze::{FreezeLock, FreezeReadGuard, FreezeWriteGuard};
66+
6467
mod mode {
6568
use super::Ordering;
6669
use std::sync::atomic::AtomicU8;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
use crate::sync::{AtomicBool, ReadGuard, RwLock, WriteGuard};
2+
#[cfg(parallel_compiler)]
3+
use crate::sync::{DynSend, DynSync};
4+
use std::{
5+
cell::UnsafeCell,
6+
marker::PhantomData,
7+
ops::{Deref, DerefMut},
8+
sync::atomic::Ordering,
9+
};
10+
11+
/// A type which allows mutation using a lock until
12+
/// the value is frozen and can be accessed lock-free.
13+
///
14+
/// Unlike `RwLock`, it can be used to prevent mutation past a point.
15+
#[derive(Default)]
16+
pub struct FreezeLock<T> {
17+
data: UnsafeCell<T>,
18+
frozen: AtomicBool,
19+
20+
/// This lock protects writes to the `data` and `frozen` fields.
21+
lock: RwLock<()>,
22+
}
23+
24+
#[cfg(parallel_compiler)]
25+
unsafe impl<T: DynSync + DynSend> DynSync for FreezeLock<T> {}
26+
27+
impl<T> FreezeLock<T> {
28+
#[inline]
29+
pub fn new(value: T) -> Self {
30+
Self { data: UnsafeCell::new(value), frozen: AtomicBool::new(false), lock: RwLock::new(()) }
31+
}
32+
33+
#[inline]
34+
pub fn read(&self) -> FreezeReadGuard<'_, T> {
35+
FreezeReadGuard {
36+
_lock_guard: if self.frozen.load(Ordering::Acquire) {
37+
None
38+
} else {
39+
Some(self.lock.read())
40+
},
41+
lock: self,
42+
}
43+
}
44+
45+
#[inline]
46+
#[track_caller]
47+
pub fn write(&self) -> FreezeWriteGuard<'_, T> {
48+
let _lock_guard = self.lock.write();
49+
// Use relaxed ordering since we're in the write lock.
50+
assert!(!self.frozen.load(Ordering::Relaxed), "still mutable");
51+
FreezeWriteGuard { _lock_guard, lock: self, marker: PhantomData }
52+
}
53+
54+
#[inline]
55+
pub fn freeze(&self) -> &T {
56+
if !self.frozen.load(Ordering::Acquire) {
57+
// Get the lock to ensure no concurrent writes and that we release the latest write.
58+
let _lock = self.lock.write();
59+
self.frozen.store(true, Ordering::Release);
60+
}
61+
62+
// SAFETY: This is frozen so the data cannot be modified and shared access is sound.
63+
unsafe { &*self.data.get() }
64+
}
65+
}
66+
67+
/// A guard holding shared access to a `FreezeLock` which is in a locked state or frozen.
68+
#[must_use = "if unused the FreezeLock may immediately unlock"]
69+
pub struct FreezeReadGuard<'a, T> {
70+
_lock_guard: Option<ReadGuard<'a, ()>>,
71+
lock: &'a FreezeLock<T>,
72+
}
73+
74+
impl<'a, T: 'a> Deref for FreezeReadGuard<'a, T> {
75+
type Target = T;
76+
#[inline]
77+
fn deref(&self) -> &T {
78+
// SAFETY: If `lock` is not frozen, `_lock_guard` holds the lock to the `UnsafeCell` so
79+
// this has shared access until the `FreezeReadGuard` is dropped. If `lock` is frozen,
80+
// the data cannot be modified and shared access is sound.
81+
unsafe { &*self.lock.data.get() }
82+
}
83+
}
84+
85+
/// A guard holding mutable access to a `FreezeLock` which is in a locked state or frozen.
86+
#[must_use = "if unused the FreezeLock may immediately unlock"]
87+
pub struct FreezeWriteGuard<'a, T> {
88+
_lock_guard: WriteGuard<'a, ()>,
89+
lock: &'a FreezeLock<T>,
90+
marker: PhantomData<&'a mut T>,
91+
}
92+
93+
impl<'a, T: 'a> Deref for FreezeWriteGuard<'a, T> {
94+
type Target = T;
95+
#[inline]
96+
fn deref(&self) -> &T {
97+
// SAFETY: `self._lock_guard` holds the lock to the `UnsafeCell` so this has shared access.
98+
unsafe { &*self.lock.data.get() }
99+
}
100+
}
101+
102+
impl<'a, T: 'a> DerefMut for FreezeWriteGuard<'a, T> {
103+
#[inline]
104+
fn deref_mut(&mut self) -> &mut T {
105+
// SAFETY: `self._lock_guard` holds the lock to the `UnsafeCell` so this has mutable access.
106+
unsafe { &mut *self.lock.data.get() }
107+
}
108+
}

compiler/rustc_hir_analysis/src/lib.rs

+4
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,10 @@ pub fn check_crate(tcx: TyCtxt<'_>) -> Result<(), ErrorGuaranteed> {
237237
tcx.hir().for_each_module(|module| tcx.ensure().check_mod_item_types(module))
238238
});
239239

240+
// Freeze definitions as we don't add new ones at this point. This improves performance by
241+
// allowing lock-free access to them.
242+
tcx.untracked().definitions.freeze();
243+
240244
// FIXME: Remove this when we implement creating `DefId`s
241245
// for anon constants during their parents' typeck.
242246
// Typeck all body owners in parallel will produce queries

compiler/rustc_interface/src/queries.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ use rustc_codegen_ssa::traits::CodegenBackend;
77
use rustc_codegen_ssa::CodegenResults;
88
use rustc_data_structures::steal::Steal;
99
use rustc_data_structures::svh::Svh;
10-
use rustc_data_structures::sync::{AppendOnlyIndexVec, Lrc, OnceLock, RwLock, WorkerLocal};
10+
use rustc_data_structures::sync::{
11+
AppendOnlyIndexVec, FreezeLock, Lrc, OnceLock, RwLock, WorkerLocal,
12+
};
1113
use rustc_hir::def_id::{StableCrateId, CRATE_DEF_ID, LOCAL_CRATE};
1214
use rustc_hir::definitions::Definitions;
1315
use rustc_incremental::DepGraphFuture;
@@ -197,7 +199,7 @@ impl<'tcx> Queries<'tcx> {
197199
self.codegen_backend().metadata_loader(),
198200
stable_crate_id,
199201
)) as _);
200-
let definitions = RwLock::new(Definitions::new(stable_crate_id));
202+
let definitions = FreezeLock::new(Definitions::new(stable_crate_id));
201203
let source_span = AppendOnlyIndexVec::new();
202204
let _id = source_span.push(krate.spans.inner_span);
203205
debug_assert_eq!(_id, CRATE_DEF_ID);

compiler/rustc_middle/src/hir/map/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1207,7 +1207,7 @@ pub(super) fn crate_hash(tcx: TyCtxt<'_>, _: LocalCrate) -> Svh {
12071207
source_file_names.hash_stable(&mut hcx, &mut stable_hasher);
12081208
debugger_visualizers.hash_stable(&mut hcx, &mut stable_hasher);
12091209
if tcx.sess.opts.incremental_relative_spans() {
1210-
let definitions = tcx.definitions_untracked();
1210+
let definitions = tcx.untracked().definitions.freeze();
12111211
let mut owner_spans: Vec<_> = krate
12121212
.owners
12131213
.iter_enumerated()

compiler/rustc_middle/src/ty/context.rs

+10-10
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ use rustc_data_structures::profiling::SelfProfilerRef;
3939
use rustc_data_structures::sharded::{IntoPointer, ShardedHashMap};
4040
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
4141
use rustc_data_structures::steal::Steal;
42-
use rustc_data_structures::sync::{self, Lock, Lrc, MappedReadGuard, ReadGuard, WorkerLocal};
42+
use rustc_data_structures::sync::{
43+
self, FreezeReadGuard, Lock, Lrc, MappedReadGuard, ReadGuard, WorkerLocal,
44+
};
4345
use rustc_data_structures::unord::UnordSet;
4446
use rustc_errors::{
4547
DecorateLint, DiagnosticBuilder, DiagnosticMessage, ErrorGuaranteed, MultiSpan,
@@ -964,8 +966,8 @@ impl<'tcx> TyCtxt<'tcx> {
964966
i += 1;
965967
}
966968

967-
// Leak a read lock once we finish iterating on definitions, to prevent adding new ones.
968-
definitions.leak();
969+
// Freeze definitions once we finish iterating on them, to prevent adding new ones.
970+
definitions.freeze();
969971
})
970972
}
971973

@@ -974,10 +976,9 @@ impl<'tcx> TyCtxt<'tcx> {
974976
// definitions change.
975977
self.dep_graph.read_index(DepNodeIndex::FOREVER_RED_NODE);
976978

977-
// Leak a read lock once we start iterating on definitions, to prevent adding new ones
979+
// Freeze definitions once we start iterating on them, to prevent adding new ones
978980
// while iterating. If some query needs to add definitions, it should be `ensure`d above.
979-
let definitions = self.untracked.definitions.leak();
980-
definitions.def_path_table()
981+
self.untracked.definitions.freeze().def_path_table()
981982
}
982983

983984
pub fn def_path_hash_to_def_index_map(
@@ -986,10 +987,9 @@ impl<'tcx> TyCtxt<'tcx> {
986987
// Create a dependency to the crate to be sure we re-execute this when the amount of
987988
// definitions change.
988989
self.ensure().hir_crate(());
989-
// Leak a read lock once we start iterating on definitions, to prevent adding new ones
990+
// Freeze definitions once we start iterating on them, to prevent adding new ones
990991
// while iterating. If some query needs to add definitions, it should be `ensure`d above.
991-
let definitions = self.untracked.definitions.leak();
992-
definitions.def_path_hash_to_def_index_map()
992+
self.untracked.definitions.freeze().def_path_hash_to_def_index_map()
993993
}
994994

995995
/// Note that this is *untracked* and should only be used within the query
@@ -1006,7 +1006,7 @@ impl<'tcx> TyCtxt<'tcx> {
10061006
/// Note that this is *untracked* and should only be used within the query
10071007
/// system if the result is otherwise tracked through queries
10081008
#[inline]
1009-
pub fn definitions_untracked(self) -> ReadGuard<'tcx, Definitions> {
1009+
pub fn definitions_untracked(self) -> FreezeReadGuard<'tcx, Definitions> {
10101010
self.untracked.definitions.read()
10111011
}
10121012

compiler/rustc_session/src/cstore.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use crate::utils::NativeLibKind;
77
use crate::Session;
88
use rustc_ast as ast;
99
use rustc_data_structures::owned_slice::OwnedSlice;
10-
use rustc_data_structures::sync::{self, AppendOnlyIndexVec, RwLock};
10+
use rustc_data_structures::sync::{self, AppendOnlyIndexVec, FreezeLock, RwLock};
1111
use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, StableCrateId, LOCAL_CRATE};
1212
use rustc_hir::definitions::{DefKey, DefPath, DefPathHash, Definitions};
1313
use rustc_span::hygiene::{ExpnHash, ExpnId};
@@ -261,5 +261,5 @@ pub struct Untracked {
261261
pub cstore: RwLock<Box<CrateStoreDyn>>,
262262
/// Reference span for definitions.
263263
pub source_span: AppendOnlyIndexVec<LocalDefId, Span>,
264-
pub definitions: RwLock<Definitions>,
264+
pub definitions: FreezeLock<Definitions>,
265265
}

0 commit comments

Comments
 (0)