Unnamed repository; edit this file 'description' to name the repository.
Do not check solver's cache validity on every access
This should help speed.
Instead, we do it only when an `Interner` is constructed, since an interner cannot be held across revisions.
There is a problem, though: an interner *can* be held across different databases. To solve that, we also reinit the cache when attaching databases, and take the assumption that everything significant (i.e. that can access the cache) will need to attach the db. This is somewhat risky, but we'll take it for the speed.
Theoretically we could *only* reinit on db attach, but it's less risky this way, and with-crate interner construction is rare.
| -rw-r--r-- | crates/hir-ty/src/next_solver/infer/mod.rs | 4 | ||||
| -rw-r--r-- | crates/hir-ty/src/next_solver/interner.rs | 42 |
2 files changed, 38 insertions, 8 deletions
diff --git a/crates/hir-ty/src/next_solver/infer/mod.rs b/crates/hir-ty/src/next_solver/infer/mod.rs index a6957c66ff..fa7aa51876 100644 --- a/crates/hir-ty/src/next_solver/infer/mod.rs +++ b/crates/hir-ty/src/next_solver/infer/mod.rs @@ -372,6 +372,10 @@ impl<'db> InferCtxtBuilder<'db> { } pub fn build(&mut self, typing_mode: TypingMode<'db>) -> InferCtxt<'db> { + // We do not allow creating an InferCtxt for an Interner without a crate, because this means + // an interner without a crate cannot access the cache, therefore constructing it doesn't need + // to reinit the cache, and we construct a lot of no-crate interners. + self.interner.expect_crate(); let InferCtxtBuilder { interner } = *self; InferCtxt { interner, diff --git a/crates/hir-ty/src/next_solver/interner.rs b/crates/hir-ty/src/next_solver/interner.rs index 1078a6af42..9ad1b3b712 100644 --- a/crates/hir-ty/src/next_solver/interner.rs +++ b/crates/hir-ty/src/next_solver/interner.rs @@ -327,6 +327,7 @@ unsafe impl Sync for DbInterner<'_> {} impl<'db> DbInterner<'db> { // FIXME(next-solver): remove this method pub fn conjure() -> DbInterner<'db> { + // Here we can not reinit the cache since we do that when we attach the db. crate::with_attached_db(|db| DbInterner { db: unsafe { std::mem::transmute::<&dyn HirDatabase, &'db dyn HirDatabase>(db) }, krate: None, @@ -339,10 +340,13 @@ impl<'db> DbInterner<'db> { /// /// Elaboration is a special kind: it needs lang items (for `Sized`), therefore it needs `new_with()`. pub fn new_no_crate(db: &'db dyn HirDatabase) -> Self { + // We do not reinit the cache here, since anything accessing the cache needs an InferCtxt, + // and we panic when trying to construct an InferCtxt for an Interner without a crate. DbInterner { db, krate: None, lang_items: None } } pub fn new_with(db: &'db dyn HirDatabase, krate: Crate) -> DbInterner<'db> { + tls_cache::reinit_cache(db); DbInterner { db, krate: Some(krate), @@ -1120,7 +1124,8 @@ impl<'db> Interner for DbInterner<'db> { self, f: impl FnOnce(&mut rustc_type_ir::search_graph::GlobalCache<Self>) -> R, ) -> R { - tls_cache::with_cache(self.db, f) + // We make sure to reinit the cache when constructing the Interner. + tls_cache::borrow_assume_valid(self.db, f) } fn canonical_param_env_cache_get_or_insert<R>( @@ -2475,6 +2480,7 @@ mod tls_db { } let _guard = DbGuard::new(self, db); + super::tls_cache::reinit_cache(db); op() } @@ -2497,10 +2503,14 @@ mod tls_db { #[inline] fn drop(&mut self) { self.state.database.set(self.prev); + if let Some(prev) = self.prev { + super::tls_cache::reinit_cache(unsafe { prev.as_ref() }); + } } } let _guard = DbGuard::new(self, db); + super::tls_cache::reinit_cache(db); op() } @@ -2555,22 +2565,38 @@ mod tls_cache { static GLOBAL_CACHE: RefCell<Option<Cache>> = const { RefCell::new(None) }; } - pub(super) fn with_cache<'db, T>( - db: &'db dyn HirDatabase, - f: impl FnOnce(&mut GlobalCache<DbInterner<'db>>) -> T, - ) -> T { + pub(super) fn reinit_cache(db: &dyn HirDatabase) { GLOBAL_CACHE.with_borrow_mut(|handle| { let (db_nonce, revision) = db.nonce_and_revision(); - let handle = match handle { + match handle { Some(handle) => { if handle.revision != revision || db_nonce != handle.db_nonce { *handle = Cache { cache: GlobalCache::default(), revision, db_nonce }; } - handle } - None => handle.insert(Cache { cache: GlobalCache::default(), revision, db_nonce }), + None => *handle = Some(Cache { cache: GlobalCache::default(), revision, db_nonce }), + } + }) + } + + pub(super) fn borrow_assume_valid<'db, T>( + db: &'db dyn HirDatabase, + f: impl FnOnce(&mut GlobalCache<DbInterner<'db>>) -> T, + ) -> T { + if cfg!(debug_assertions) { + let get_state = || { + GLOBAL_CACHE.with_borrow(|handle| { + handle.as_ref().map(|handle| (handle.db_nonce, handle.revision)) + }) }; + let old_state = get_state(); + reinit_cache(db); + let new_state = get_state(); + assert_eq!(old_state, new_state, "you assumed the cache is valid!"); + } + GLOBAL_CACHE.with_borrow_mut(|handle| { + let handle = handle.as_mut().expect("you assumed the cache is valid!"); // SAFETY: No idea f(unsafe { std::mem::transmute::< |