From 6a0a5337e1a34b5253021ab3b379d1c7546fe935 Mon Sep 17 00:00:00 2001 From: DonIsaac <22823424+DonIsaac@users.noreply.github.com> Date: Fri, 11 Oct 2024 02:11:27 +0000 Subject: [PATCH] refactor(linter): move module cache logic out of Runtime (#6438) Pure refactor. Makes `Runtime` easier to reason about. --- crates/oxc_linter/src/service/mod.rs | 3 +- crates/oxc_linter/src/service/module_cache.rs | 95 ++++++++++++++++++- crates/oxc_linter/src/service/runtime.rs | 76 +++------------ 3 files changed, 103 insertions(+), 71 deletions(-) diff --git a/crates/oxc_linter/src/service/mod.rs b/crates/oxc_linter/src/service/mod.rs index 1566db3fb0a70..93d4ac127757c 100644 --- 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 index bf2e7d6a89666..069c82ad38193 100644 --- 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 /// and the solution is copied here to fix the issue. -pub(super) type CacheState = Mutex, Arc<(Mutex, Condvar)>>>; +type CacheState = Mutex, Arc<(Mutex, 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, ModuleState>; +type ModuleMap = DashMap, ModuleState>; #[derive(Clone)] pub(super) enum ModuleState { Resolved(Arc), Ignored, } + +#[derive(Default)] +pub(super) struct ModuleCache { + cache_state: Arc, + modules: ModuleMap, +} + +impl ModuleCache { + #[inline] + pub fn get(&self, path: &Path) -> Option, 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) { + 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 index 3dab419dd9bf1..5c8da63b94c27 100644 --- 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,7 +22,10 @@ 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, @@ -30,8 +33,7 @@ pub struct Runtime { pub(super) paths: FxHashSet>, pub(super) linter: Linter, pub(super) resolver: Option, - 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)); } }