Skip to content

Make OnDiskCache thread-safer #49396

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 14, 2018
Merged

Make OnDiskCache thread-safer #49396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 3 additions & 3 deletions src/librustc/ty/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ use rustc_data_structures::indexed_vec::IndexVec;
use rustc_data_structures::sync::{Lrc, Lock};
use std::any::Any;
use std::borrow::Borrow;
use std::cell::{Cell, RefCell};
use std::cell::Cell;
use std::cmp::Ordering;
use std::collections::hash_map::{self, Entry};
use std::hash::{Hash, Hasher};
Expand Down Expand Up @@ -867,7 +867,7 @@ pub struct GlobalCtxt<'tcx> {
maybe_unused_extern_crates: Vec<(DefId, Span)>,

// Internal cache for metadata decoding. No need to track deps on this.
pub rcache: RefCell<FxHashMap<ty::CReaderCacheKey, Ty<'tcx>>>,
pub rcache: Lock<FxHashMap<ty::CReaderCacheKey, Ty<'tcx>>>,
Copy link
Member

Choose a reason for hiding this comment

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

This field is also used very often, also during metadata encoding. Might be a good candidate for a thread-local cache in the future.


/// Caches the results of trait selection. This cache is used
/// for things that do not have to do with the parameters in scope.
Expand Down Expand Up @@ -1263,7 +1263,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
hir,
def_path_hash_to_def_id,
maps: maps::Maps::new(providers),
rcache: RefCell::new(FxHashMap()),
rcache: Lock::new(FxHashMap()),
selection_cache: traits::SelectionCache::new(),
evaluation_cache: traits::EvaluationCache::new(),
crate_name: Symbol::intern(crate_name),
Expand Down
45 changes: 22 additions & 23 deletions src/librustc/ty/maps/on_disk_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use hir::map::definitions::DefPathHash;
use ich::{CachingCodemapView, Fingerprint};
use mir::{self, interpret};
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::sync::Lrc;
use rustc_data_structures::sync::{Lrc, Lock, HashMapExt, Once};
use rustc_data_structures::indexed_vec::{IndexVec, Idx};
use rustc_serialize::{Decodable, Decoder, Encodable, Encoder, opaque,
SpecializedDecoder, SpecializedEncoder,
Expand Down Expand Up @@ -57,17 +57,17 @@ pub struct OnDiskCache<'sess> {

// This field collects all Diagnostics emitted during the current
// compilation session.
current_diagnostics: RefCell<FxHashMap<DepNodeIndex, Vec<Diagnostic>>>,
current_diagnostics: Lock<FxHashMap<DepNodeIndex, Vec<Diagnostic>>>,

prev_cnums: Vec<(u32, String, CrateDisambiguator)>,
cnum_map: RefCell<Option<IndexVec<CrateNum, Option<CrateNum>>>>,
cnum_map: Once<IndexVec<CrateNum, Option<CrateNum>>>,

codemap: &'sess CodeMap,
file_index_to_stable_id: FxHashMap<FileMapIndex, StableFilemapId>,

// These two fields caches that are populated lazily during decoding.
file_index_to_file: RefCell<FxHashMap<FileMapIndex, Lrc<FileMap>>>,
synthetic_expansion_infos: RefCell<FxHashMap<AbsoluteBytePos, SyntaxContext>>,
file_index_to_file: Lock<FxHashMap<FileMapIndex, Lrc<FileMap>>>,
synthetic_expansion_infos: Lock<FxHashMap<AbsoluteBytePos, SyntaxContext>>,
Copy link
Member

Choose a reason for hiding this comment

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

These two fields are locked for every span value that's encoded. I wonder if we can do better here. They are just simple caches.


// A map from dep-node to the position of the cached query result in
// `serialized_data`.
Expand Down Expand Up @@ -140,14 +140,14 @@ impl<'sess> OnDiskCache<'sess> {
OnDiskCache {
serialized_data: data,
file_index_to_stable_id: footer.file_index_to_stable_id,
file_index_to_file: RefCell::new(FxHashMap()),
file_index_to_file: Lock::new(FxHashMap()),
prev_cnums: footer.prev_cnums,
cnum_map: RefCell::new(None),
cnum_map: Once::new(),
codemap: sess.codemap(),
current_diagnostics: RefCell::new(FxHashMap()),
current_diagnostics: Lock::new(FxHashMap()),
query_result_index: footer.query_result_index.into_iter().collect(),
prev_diagnostics_index: footer.diagnostics_index.into_iter().collect(),
synthetic_expansion_infos: RefCell::new(FxHashMap()),
synthetic_expansion_infos: Lock::new(FxHashMap()),
interpret_alloc_cache: RefCell::new(FxHashMap::default()),
interpret_alloc_size: RefCell::new(FxHashMap::default()),
}
Expand All @@ -157,14 +157,14 @@ impl<'sess> OnDiskCache<'sess> {
OnDiskCache {
serialized_data: Vec::new(),
file_index_to_stable_id: FxHashMap(),
file_index_to_file: RefCell::new(FxHashMap()),
file_index_to_file: Lock::new(FxHashMap()),
prev_cnums: vec![],
cnum_map: RefCell::new(None),
cnum_map: Once::new(),
codemap,
current_diagnostics: RefCell::new(FxHashMap()),
current_diagnostics: Lock::new(FxHashMap()),
query_result_index: FxHashMap(),
prev_diagnostics_index: FxHashMap(),
synthetic_expansion_infos: RefCell::new(FxHashMap()),
synthetic_expansion_infos: Lock::new(FxHashMap()),
interpret_alloc_cache: RefCell::new(FxHashMap::default()),
interpret_alloc_size: RefCell::new(FxHashMap::default()),
}
Expand Down Expand Up @@ -383,18 +383,16 @@ impl<'sess> OnDiskCache<'sess> {
return None
};

// Initialize the cnum_map if it is not initialized yet.
if self.cnum_map.borrow().is_none() {
let mut cnum_map = self.cnum_map.borrow_mut();
*cnum_map = Some(Self::compute_cnum_map(tcx, &self.prev_cnums[..]));
}
let cnum_map = self.cnum_map.borrow();
// Initialize the cnum_map using the value from the thread which finishes the closure first
self.cnum_map.init_nonlocking_same(|| {
Self::compute_cnum_map(tcx, &self.prev_cnums[..])
});

let mut decoder = CacheDecoder {
tcx,
opaque: opaque::Decoder::new(&self.serialized_data[..], pos.to_usize()),
codemap: self.codemap,
cnum_map: cnum_map.as_ref().unwrap(),
cnum_map: self.cnum_map.get(),
file_index_to_file: &self.file_index_to_file,
file_index_to_stable_id: &self.file_index_to_stable_id,
synthetic_expansion_infos: &self.synthetic_expansion_infos,
Expand Down Expand Up @@ -458,8 +456,8 @@ struct CacheDecoder<'a, 'tcx: 'a, 'x> {
opaque: opaque::Decoder<'x>,
codemap: &'x CodeMap,
cnum_map: &'x IndexVec<CrateNum, Option<CrateNum>>,
synthetic_expansion_infos: &'x RefCell<FxHashMap<AbsoluteBytePos, SyntaxContext>>,
file_index_to_file: &'x RefCell<FxHashMap<FileMapIndex, Lrc<FileMap>>>,
synthetic_expansion_infos: &'x Lock<FxHashMap<AbsoluteBytePos, SyntaxContext>>,
file_index_to_file: &'x Lock<FxHashMap<FileMapIndex, Lrc<FileMap>>>,
file_index_to_stable_id: &'x FxHashMap<FileMapIndex, StableFilemapId>,
interpret_alloc_cache: &'x RefCell<FxHashMap<usize, interpret::AllocId>>,
interpret_alloc_size: &'x RefCell<FxHashMap<usize, usize>>,
Expand Down Expand Up @@ -557,7 +555,8 @@ impl<'a, 'tcx: 'a, 'x> ty_codec::TyDecoder<'a, 'tcx> for CacheDecoder<'a, 'tcx,
}

let ty = or_insert_with(self)?;
tcx.rcache.borrow_mut().insert(cache_key, ty);
// This may overwrite the entry, but it should overwrite with the same value
tcx.rcache.borrow_mut().insert_same(cache_key, ty);
Ok(ty)
}

Expand Down