refactor(linter): move module cache logic out of Runtime (#6438)

Pure refactor. Makes `Runtime` easier to reason about.
oxc-project · Oct 11, 2024 · 6a0a533 · 6a0a533
1 parent c18c6e9
commit 6a0a533
Show file tree

Hide file tree

Showing 3 changed files with 103 additions and 71 deletions.
diff --git a/crates/oxc_linter/src/service/mod.rs b/crates/oxc_linter/src/service/mod.rs
@@ -11,7 +11,6 @@ use rayon::{iter::ParallelBridge, prelude::ParallelIterator};
 
 use crate::Linter;
 
-use module_cache::{CacheState, CacheStateEntry, ModuleMap, ModuleState};
 use runtime::Runtime;
 
 pub struct LintServiceOptions {
@@ -86,7 +85,7 @@ impl LintService {
     }
 
     pub fn number_of_dependencies(&self) -> usize {
-        self.runtime.module_map.len() - self.runtime.paths.len()
+        self.runtime.modules.len() - self.runtime.paths.len()
     }
 
     /// # Panics

diff --git a/crates/oxc_linter/src/service/module_cache.rs b/crates/oxc_linter/src/service/module_cache.rs
@@ -3,7 +3,7 @@ use std::{
     sync::{Arc, Condvar, Mutex},
 };
 
-use dashmap::DashMap;
+use dashmap::{mapref::one::Ref, DashMap};
 use oxc_semantic::ModuleRecord;
 use rustc_hash::FxHashMap;
 
@@ -16,19 +16,106 @@ use rustc_hash::FxHashMap;
 ///
 /// See the "problem section" in <https://medium.com/@polyglot_factotum/rust-concurrency-patterns-condvars-and-locks-e278f18db74f>
 /// and the solution is copied here to fix the issue.
-pub(super) type CacheState = Mutex<FxHashMap<Box<Path>, Arc<(Mutex<CacheStateEntry>, Condvar)>>>;
+type CacheState = Mutex<FxHashMap<Box<Path>, Arc<(Mutex<CacheStateEntry>, Condvar)>>>;
 
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
-pub(super) enum CacheStateEntry {
+enum CacheStateEntry {
     ReadyToConstruct,
     PendingStore(usize),
 }
 
 /// Keyed by canonicalized path
-pub(super) type ModuleMap = DashMap<Box<Path>, ModuleState>;
+type ModuleMap = DashMap<Box<Path>, ModuleState>;
 
 #[derive(Clone)]
 pub(super) enum ModuleState {
     Resolved(Arc<ModuleRecord>),
     Ignored,
 }
+
+#[derive(Default)]
+pub(super) struct ModuleCache {
+    cache_state: Arc<CacheState>,
+    modules: ModuleMap,
+}
+
+impl ModuleCache {
+    #[inline]
+    pub fn get(&self, path: &Path) -> Option<Ref<'_, Box<Path>, ModuleState>> {
+        self.modules.get(path)
+    }
+
+    #[inline]
+    pub fn len(&self) -> usize {
+        self.modules.len()
+    }
+
+    pub(super) fn init_cache_state(&self, path: &Path) -> bool {
+        let (lock, cvar) = {
+            let mut state_map = self.cache_state.lock().expect("Failed to lock cache state");
+            &*Arc::clone(state_map.entry(path.to_path_buf().into_boxed_path()).or_insert_with(
+                || Arc::new((Mutex::new(CacheStateEntry::ReadyToConstruct), Condvar::new())),
+            ))
+        };
+        let mut state = cvar
+            .wait_while(lock.lock().expect("Failed lock inner cache state"), |state| {
+                matches!(*state, CacheStateEntry::PendingStore(_))
+            })
+            .unwrap();
+
+        let cache_hit = if self.modules.contains_key(path) {
+            true
+        } else {
+            let i = if let CacheStateEntry::PendingStore(i) = *state { i } else { 0 };
+            *state = CacheStateEntry::PendingStore(i + 1);
+            false
+        };
+
+        if *state == CacheStateEntry::ReadyToConstruct {
+            cvar.notify_one();
+        }
+
+        drop(state);
+        cache_hit
+    }
+
+    /// # Panics
+    /// If a cache entry for `path` does not exist. You must call `init_cache_state` first.
+    pub(super) fn add_resolved_module(&self, path: &Path, module_record: Arc<ModuleRecord>) {
+        self.modules
+            .insert(path.to_path_buf().into_boxed_path(), ModuleState::Resolved(module_record));
+
+        self.update_cache_state(path);
+    }
+
+    /// # Panics
+    /// If a cache entry for `path` does not exist. You must call `init_cache_state` first.
+    pub(super) fn ignore_path(&self, path: &Path) {
+        self.modules.insert(path.to_path_buf().into_boxed_path(), ModuleState::Ignored);
+        self.update_cache_state(path);
+    }
+
+    /// # Panics
+    /// If a cache entry for `path` does not exist. You must call `init_cache_state` first.
+    fn update_cache_state(&self, path: &Path) {
+        let (lock, cvar) = {
+            let mut state_map = self.cache_state.lock().expect("Failed to lock cache state");
+            &*Arc::clone(
+                state_map
+                    .get_mut(path)
+                    .expect("Entry in http-cache state to have been previously inserted"),
+            )
+        };
+        let mut state = lock.lock().expect("Failed lock inner cache state");
+        if let CacheStateEntry::PendingStore(i) = *state {
+            let new = i - 1;
+            if new == 0 {
+                *state = CacheStateEntry::ReadyToConstruct;
+                // Notify the next thread waiting in line, if there is any.
+                cvar.notify_one();
+            } else {
+                *state = CacheStateEntry::PendingStore(new);
+            }
+        }
+    }
+}
diff --git a/crates/oxc_linter/src/service/runtime.rs b/crates/oxc_linter/src/service/runtime.rs
@@ -4,7 +4,7 @@ use std::{
     fs,
     path::{Path, PathBuf},
     rc::Rc,
-    sync::{Arc, Condvar, Mutex},
+    sync::Arc,
 };
 
 use oxc_allocator::Allocator;
@@ -22,16 +22,18 @@ use crate::{
     Fixer, Linter, Message,
 };
 
-use super::{CacheState, CacheStateEntry, LintServiceOptions, ModuleMap, ModuleState};
+use super::{
+    module_cache::{ModuleCache, ModuleState},
+    LintServiceOptions,
+};
 
 pub struct Runtime {
     pub(super) cwd: Box<Path>,
     /// All paths to lint
     pub(super) paths: FxHashSet<Box<Path>>,
     pub(super) linter: Linter,
     pub(super) resolver: Option<Resolver>,
-    pub(super) module_map: ModuleMap,
-    pub(super) cache_state: CacheState,
+    pub(super) modules: ModuleCache,
 }
 
 impl Runtime {
@@ -44,8 +46,7 @@ impl Runtime {
             paths: options.paths.iter().cloned().collect(),
             linter,
             resolver,
-            module_map: ModuleMap::default(),
-            cache_state: CacheState::default(),
+            modules: ModuleCache::default(),
         }
     }
 
@@ -216,11 +217,7 @@ impl Runtime {
         let module_record = semantic_builder.module_record();
 
         if self.resolver.is_some() {
-            self.module_map.insert(
-                path.to_path_buf().into_boxed_path(),
-                ModuleState::Resolved(Arc::clone(&module_record)),
-            );
-            self.update_cache_state(path);
+            self.modules.add_resolved_module(path, Arc::clone(&module_record));
 
             // Retrieve all dependency modules from this module.
             let dir = path.parent().unwrap();
@@ -235,7 +232,7 @@ impl Runtime {
                 .for_each_with(tx_error, |tx_error, (specifier, resolution)| {
                     let path = resolution.path();
                     self.process_path(path, tx_error);
-                    let Some(target_module_record_ref) = self.module_map.get(path) else {
+                    let Some(target_module_record_ref) = self.modules.get(path) else {
                         return;
                     };
                     let ModuleState::Resolved(target_module_record) =
@@ -301,60 +298,9 @@ impl Runtime {
             return false;
         }
 
-        let (lock, cvar) = {
-            let mut state_map = self.cache_state.lock().unwrap();
-            &*Arc::clone(state_map.entry(path.to_path_buf().into_boxed_path()).or_insert_with(
-                || Arc::new((Mutex::new(CacheStateEntry::ReadyToConstruct), Condvar::new())),
-            ))
-        };
-        let mut state = cvar
-            .wait_while(lock.lock().unwrap(), |state| {
-                matches!(*state, CacheStateEntry::PendingStore(_))
-            })
-            .unwrap();
-
-        let cache_hit = if self.module_map.contains_key(path) {
-            true
-        } else {
-            let i = if let CacheStateEntry::PendingStore(i) = *state { i } else { 0 };
-            *state = CacheStateEntry::PendingStore(i + 1);
-            false
-        };
-
-        if *state == CacheStateEntry::ReadyToConstruct {
-            cvar.notify_one();
-        }
-
-        drop(state);
-        cache_hit
+        self.modules.init_cache_state(path)
     }
-
-    fn update_cache_state(&self, path: &Path) {
-        let (lock, cvar) = {
-            let mut state_map = self.cache_state.lock().unwrap();
-            &*Arc::clone(
-                state_map
-                    .get_mut(path)
-                    .expect("Entry in http-cache state to have been previously inserted"),
-            )
-        };
-        let mut state = lock.lock().unwrap();
-        if let CacheStateEntry::PendingStore(i) = *state {
-            let new = i - 1;
-            if new == 0 {
-                *state = CacheStateEntry::ReadyToConstruct;
-                // Notify the next thread waiting in line, if there is any.
-                cvar.notify_one();
-            } else {
-                *state = CacheStateEntry::PendingStore(new);
-            }
-        }
-    }
-
     fn ignore_path(&self, path: &Path) {
-        if self.resolver.is_some() {
-            self.module_map.insert(path.to_path_buf().into_boxed_path(), ModuleState::Ignored);
-            self.update_cache_state(path);
-        }
+        self.resolver.is_some().then(|| self.modules.ignore_path(path));
     }
 }