From 042a497626df342042ad96f549fe82fd6b7f79ef Mon Sep 17 00:00:00 2001 From: mattcompiles Date: Fri, 15 Nov 2024 10:12:52 +1100 Subject: [PATCH 1/2] Update shared hashmap to use RwLock instead of threadlocal --- crates/atlaspack_filesystem/src/lib.rs | 6 +-- crates/atlaspack_shared_map/src/lib.rs | 57 ++++++++------------ packages/utils/node-resolver-rs/src/cache.rs | 18 +++---- 3 files changed, 32 insertions(+), 49 deletions(-) diff --git a/crates/atlaspack_filesystem/src/lib.rs b/crates/atlaspack_filesystem/src/lib.rs index ef9395648..5a5aaf43a 100644 --- a/crates/atlaspack_filesystem/src/lib.rs +++ b/crates/atlaspack_filesystem/src/lib.rs @@ -2,7 +2,7 @@ use std::path::Path; use std::path::PathBuf; use std::sync::Arc; -use atlaspack_shared_map::ThreadLocalHashMap; +use atlaspack_shared_map::SharedHashMap; /// In-memory file-system for testing pub mod in_memory_file_system; @@ -17,9 +17,7 @@ pub mod os_file_system; /// This should be `OsFileSystem` for non-testing environments and `InMemoryFileSystem` for testing. pub type FileSystemRef = Arc; -pub type DefaultHasher = xxhash_rust::xxh3::Xxh3Builder; - -pub type FileSystemRealPathCache = ThreadLocalHashMap, DefaultHasher>; +pub type FileSystemRealPathCache = SharedHashMap>; /// Trait abstracting file-system operations /// . diff --git a/crates/atlaspack_shared_map/src/lib.rs b/crates/atlaspack_shared_map/src/lib.rs index e339462f1..624a2a642 100644 --- a/crates/atlaspack_shared_map/src/lib.rs +++ b/crates/atlaspack_shared_map/src/lib.rs @@ -1,25 +1,16 @@ -use std::{ - borrow::Borrow, - cell::RefCell, - collections::HashMap, - hash::BuildHasher, - ops::{Deref, DerefMut}, -}; +use std::{borrow::Borrow, collections::HashMap, hash::BuildHasher, sync::RwLock}; -use thread_local::ThreadLocal; - -/// A thread-local hash map. -/// This is to be used within a shared structure but disabling the need for -/// actually sharing data between threads. -pub struct ThreadLocalHashMap< +/// A HashMap that allows sharing of values between threads +/// It is currently implemented using RwLock +pub struct SharedHashMap< K: Send + Eq, V: Send + Clone, H: Send + Default + BuildHasher = xxhash_rust::xxh3::Xxh3Builder, > { - inner: ThreadLocal>>, + inner: RwLock>, } -impl Default for ThreadLocalHashMap +impl Default for SharedHashMap where K: Send + Eq, V: Send + Clone, @@ -27,12 +18,12 @@ where { fn default() -> Self { Self { - inner: ThreadLocal::new(), + inner: RwLock::new(HashMap::with_hasher(H::default())), } } } -impl ThreadLocalHashMap +impl SharedHashMap where K: std::hash::Hash + Send + Eq, V: Send + Clone, @@ -40,7 +31,7 @@ where { pub fn new() -> Self { Self { - inner: ThreadLocal::new(), + inner: RwLock::new(HashMap::with_hasher(H::default())), } } @@ -50,21 +41,15 @@ where K: Borrow, KR: Eq + std::hash::Hash, { - let map_cell = self - .inner - .get_or(|| RefCell::new(HashMap::with_hasher(H::default()))); - - let map = map_cell.borrow(); - map.deref().get(key).cloned() + let map_cell = self.inner.read().unwrap(); + let map = map_cell; + map.get(key).cloned() } pub fn insert(&self, key: K, value: V) { - let map_cell = self - .inner - .get_or(|| RefCell::new(HashMap::with_hasher(H::default()))); - - let mut map = map_cell.borrow_mut(); - map.deref_mut().insert(key, value); + let map_cell = self.inner.write().unwrap(); + let mut map = map_cell; + map.insert(key, value); } } @@ -75,15 +60,15 @@ mod test { use super::*; #[test] - fn test_thread_local_hash_map() { - let map = ThreadLocalHashMap::::new(); + fn test_shared_hash_map() { + let map = SharedHashMap::::new(); map.insert("key".to_string(), "value".to_string()); assert_eq!(map.get("key"), Some("value".to_string())); } #[test] - fn test_multiple_thread_local_hash_map() { - let map = Arc::new(ThreadLocalHashMap::::new()); + fn test_multiple_shared_hash_map() { + let map = Arc::new(SharedHashMap::::new()); let (close_tx, close_rx) = std::sync::mpsc::channel(); let (tx, rx) = std::sync::mpsc::channel(); @@ -98,11 +83,11 @@ mod test { }); rx.recv().unwrap(); // wait for value to be written - // and check value is not visible on other threads + // and check value is visible on other threads std::thread::spawn({ let map = map.clone(); move || { - assert_eq!(map.get("key"), None); + assert_eq!(map.get("key"), Some("value".to_string())); } }) .join() diff --git a/packages/utils/node-resolver-rs/src/cache.rs b/packages/utils/node-resolver-rs/src/cache.rs index 1a7fc402f..ffca0419a 100644 --- a/packages/utils/node-resolver-rs/src/cache.rs +++ b/packages/utils/node-resolver-rs/src/cache.rs @@ -7,7 +7,7 @@ use std::sync::Arc; use atlaspack_core::types::File; use atlaspack_filesystem::{FileSystemRealPathCache, FileSystemRef}; -use atlaspack_shared_map::ThreadLocalHashMap; +use atlaspack_shared_map::SharedHashMap; use crate::package_json::PackageJson; use crate::package_json::SourceField; @@ -22,13 +22,13 @@ pub struct Cache { /// from our public methods so this is ok for now. FrozenMap is an append only map, which doesn't require &mut /// to insert into. Since each value is in a Box, it won't move and therefore references are stable. #[allow(clippy::type_complexity)] - packages: ThreadLocalHashMap, ResolverError>>>, + packages: SharedHashMap, ResolverError>>>, #[allow(clippy::type_complexity)] - tsconfigs: ThreadLocalHashMap, ResolverError>>>, + tsconfigs: SharedHashMap, ResolverError>>>, // In particular just the is_dir_cache spends around 8% of the time on a large project resolution // hashing paths. Instead of using a hashmap we should try a trie here. - is_dir_cache: ThreadLocalHashMap, - is_file_cache: ThreadLocalHashMap, + is_dir_cache: SharedHashMap, + is_file_cache: SharedHashMap, realpath_cache: FileSystemRealPathCache, } @@ -91,10 +91,10 @@ impl Cache { pub fn new(fs: FileSystemRef) -> Self { Self { fs, - packages: ThreadLocalHashMap::new(), - tsconfigs: ThreadLocalHashMap::new(), - is_file_cache: ThreadLocalHashMap::new(), - is_dir_cache: ThreadLocalHashMap::new(), + packages: SharedHashMap::new(), + tsconfigs: SharedHashMap::new(), + is_file_cache: SharedHashMap::new(), + is_dir_cache: SharedHashMap::new(), realpath_cache: FileSystemRealPathCache::default(), } } From f1cf7cb8a7f5892bfe1a000b0d072bfff96ba46b Mon Sep 17 00:00:00 2001 From: mattcompiles Date: Fri, 15 Nov 2024 10:14:20 +1100 Subject: [PATCH 2/2] Rename test --- crates/atlaspack_shared_map/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/atlaspack_shared_map/src/lib.rs b/crates/atlaspack_shared_map/src/lib.rs index 624a2a642..ad1e889f5 100644 --- a/crates/atlaspack_shared_map/src/lib.rs +++ b/crates/atlaspack_shared_map/src/lib.rs @@ -67,7 +67,7 @@ mod test { } #[test] - fn test_multiple_shared_hash_map() { + fn test_multiple_thread_shared_hash_map() { let map = Arc::new(SharedHashMap::::new()); let (close_tx, close_rx) = std::sync::mpsc::channel(); let (tx, rx) = std::sync::mpsc::channel();