From eafad4dfd113efc9bd33bcf5d109d3635b011c59 Mon Sep 17 00:00:00 2001 From: Boshen Date: Mon, 24 Jul 2023 20:42:09 +0800 Subject: [PATCH] perf(resolver): reduce the total number of hashes by passing the cached value around (#602) --- crates/oxc_resolver/src/cache.rs | 111 ++++++----- crates/oxc_resolver/src/lib.rs | 172 +++++++++++------- .../tests/enhanced_resolve/test/roots.rs | 2 +- 3 files changed, 162 insertions(+), 123 deletions(-) diff --git a/crates/oxc_resolver/src/cache.rs b/crates/oxc_resolver/src/cache.rs index 71aa8345b..99352defe 100644 --- a/crates/oxc_resolver/src/cache.rs +++ b/crates/oxc_resolver/src/cache.rs @@ -10,7 +10,7 @@ use rustc_hash::FxHasher; use crate::{package_json::PackageJson, FileMetadata, FileSystem, ResolveError}; pub struct Cache { - fs: Fs, + pub(crate) fs: Fs, cache: DashMap, Arc, BuildHasherDefault>, } @@ -25,62 +25,22 @@ impl Cache { Self { fs, ..Self::default() } } - pub fn is_file(&self, path: &Path) -> bool { - self.cache_value(path).is_file(&self.fs) - } - - pub fn is_dir(&self, path: &Path) -> bool { - self.cache_value(path).is_dir(&self.fs) - } - /// # Panics /// /// * Path is file but does not have a parent - pub fn dirname(&self, path: &Path) -> PathBuf { - (if self.is_file(path) { path.parent().unwrap() } else { path }).to_path_buf() + pub fn dirname(&self, cache_value: &Arc) -> Arc { + Arc::clone(if cache_value.is_file(&self.fs) { + cache_value.parent.as_ref().unwrap() + } else { + cache_value + }) } - pub fn canonicalize(&self, path: &Path) -> Option { - self.cache_value(path).symlink(&self.fs).map(Path::into_path_buf) - } - - /// Get package.json of the given path. - /// - /// # Errors - /// - /// * [ResolveError::JSON] - pub fn get_package_json(&self, path: &Path) -> Result>, ResolveError> { - self.cache_value(path).package_json(&self.fs).transpose() - } - - /// Find package.json of a path by traversing parent directories. - /// - /// # Errors - /// - /// * [ResolveError::JSON] - pub fn find_package_json(&self, path: &Path) -> Result>, ResolveError> { - let mut cache_value = self.cache_value(path); - // Go up a directory when querying a file, this avoids a file read from example.js/package.json - if cache_value.is_file(&self.fs) { - if let Some(cv) = &cache_value.parent { - cache_value = Arc::clone(cv); - } - } - let mut cache_value = Some(cache_value); - while let Some(cv) = cache_value { - if let Some(package_json) = cv.package_json(&self.fs).transpose()? { - return Ok(Some(Arc::clone(&package_json))); - } - cache_value = cv.parent.clone(); - } - Ok(None) - } - - fn cache_value(&self, path: &Path) -> Arc { + pub fn value(&self, path: &Path) -> Arc { if let Some(cache_entry) = self.cache.get(path) { return Arc::clone(cache_entry.value()); } - let parent = path.parent().map(|p| self.cache_value(p)); + let parent = path.parent().map(|p| self.value(p)); let data = Arc::new(CacheValue::new(path.to_path_buf().into_boxed_path(), parent)); self.cache.insert(path.to_path_buf().into_boxed_path(), Arc::clone(&data)); data @@ -92,7 +52,7 @@ pub struct CacheValue { path: Box, parent: Option>, meta: OnceLock>, - symlink: OnceLock>>, + symlink: OnceLock>, package_json: OnceLock, ResolveError>>>, } @@ -107,25 +67,62 @@ impl CacheValue { } } + pub fn path(&self) -> &Path { + &self.path + } + + pub fn to_path_buf(&self) -> PathBuf { + self.path.to_path_buf() + } + fn meta(&self, fs: &Fs) -> Option { *self.meta.get_or_init(|| fs.metadata(&self.path).ok()) } - fn is_file(&self, fs: &Fs) -> bool { + pub fn is_file(&self, fs: &Fs) -> bool { self.meta(fs).is_some_and(|meta| meta.is_file) } - fn is_dir(&self, fs: &Fs) -> bool { + pub fn is_dir(&self, fs: &Fs) -> bool { self.meta(fs).is_some_and(|meta| meta.is_dir) } - fn symlink(&self, fs: &Fs) -> Option> { - self.symlink - .get_or_init(|| fs.canonicalize(&self.path).map(PathBuf::into_boxed_path).ok()) - .clone() + pub fn symlink(&self, fs: &Fs) -> Option { + self.symlink.get_or_init(|| fs.canonicalize(&self.path).ok()).clone() } - fn package_json( + /// Find package.json of a path by traversing parent directories. + /// + /// # Errors + /// + /// * [ResolveError::JSON] + pub fn find_package_json( + &self, + fs: &Fs, + ) -> Result>, ResolveError> { + let mut cache_value = self; + // Go up a directory when querying a file, this avoids a file read from example.js/package.json + if cache_value.is_file(fs) { + if let Some(cv) = &cache_value.parent { + cache_value = cv.as_ref(); + } + } + let mut cache_value = Some(cache_value); + while let Some(cv) = cache_value { + if let Some(package_json) = cv.package_json(fs).transpose()? { + return Ok(Some(Arc::clone(&package_json))); + } + cache_value = cv.parent.as_deref(); + } + Ok(None) + } + + /// Get package.json of the given path. + /// + /// # Errors + /// + /// * [ResolveError::JSON] + pub fn package_json( &self, fs: &Fs, ) -> Option, ResolveError>> { diff --git a/crates/oxc_resolver/src/lib.rs b/crates/oxc_resolver/src/lib.rs index 7e58b7ca9..523010d1f 100644 --- a/crates/oxc_resolver/src/lib.rs +++ b/crates/oxc_resolver/src/lib.rs @@ -21,10 +21,11 @@ use std::{ borrow::Cow, ffi::OsStr, path::{Path, PathBuf}, + sync::Arc, }; use crate::{ - cache::Cache, + cache::{Cache, CacheValue}, file_system::FileSystemOs, package_json::PackageJson, path::PathUtil, @@ -37,7 +38,7 @@ pub use crate::{ resolution::Resolution, }; -type ResolveState = Result, ResolveError>; +type ResolveState = Result>, ResolveError>; /// Resolver with the current operating system as the file system pub type Resolver = ResolverGeneric; @@ -75,15 +76,16 @@ impl ResolverGeneric { ) -> Result { let path = path.as_ref(); let request = Request::parse(request).map_err(ResolveError::Request)?; - let path = if let Some(path) = - self.load_alias(path, request.path.as_str(), &self.options.alias)? + let cache_value = self.cache.value(path); + let cache_value = if let Some(path) = + self.load_alias(&cache_value, request.path.as_str(), &self.options.alias)? { path } else { - let result = self.require(path, &request); + let result = self.require(&cache_value, &request); if result.as_ref().is_err_and(ResolveError::is_not_found) { if let Some(path) = - self.load_alias(path, request.path.as_str(), &self.options.fallback)? + self.load_alias(&cache_value, request.path.as_str(), &self.options.fallback)? { path } else { @@ -93,7 +95,7 @@ impl ResolverGeneric { result? } }; - let path = self.load_symlink(&path).unwrap_or(path); + let path = self.load_symlink(&cache_value).unwrap_or_else(|| cache_value.to_path_buf()); Ok(Resolution { path, query: request.query.map(ToString::to_string), @@ -104,7 +106,11 @@ impl ResolverGeneric { /// require(X) from module at path Y /// X: request /// Y: path - fn require(&self, path: &Path, request: &Request) -> Result { + fn require( + &self, + cache_value: &Arc, + request: &Request, + ) -> Result, ResolveError> { match request.path { // 1. If X is a core module, // a. return the core module @@ -113,43 +119,54 @@ impl ResolverGeneric { // a. set Y to be the file system root RequestPath::Absolute(absolute_path) => { if !self.options.prefer_relative && self.options.prefer_absolute { - if let Ok(path) = self.require_path(path, absolute_path) { + if let Ok(path) = self.require_path(cache_value, absolute_path) { return Ok(path); } } - self.load_roots(path, absolute_path) + self.load_roots(cache_value, absolute_path) } // 3. If X begins with './' or '/' or '../' - RequestPath::Relative(relative_path) => self.require_relative(path, relative_path), + RequestPath::Relative(relative_path) => { + self.require_relative(cache_value, relative_path) + } // 4. If X begins with '#' - RequestPath::Hash(hash_path) => self.require_path(path, hash_path), + RequestPath::Hash(hash_path) => self.require_path(cache_value, hash_path), // a. LOAD_PACKAGE_IMPORTS(X, dirname(Y)) - RequestPath::Module(module_path) => self.require_path(path, module_path), + RequestPath::Module(module_path) => self.require_path(cache_value, module_path), } } // 3. If X begins with './' or '/' or '../' - fn require_relative(&self, path: &Path, request: &str) -> Result { - if let Some(path) = self.load_package_self(path, request)? { + fn require_relative( + &self, + cache_value: &Arc, + request: &str, + ) -> Result, ResolveError> { + if let Some(path) = self.load_package_self(cache_value, request)? { return Ok(path); } - let path = path.normalize_with(request); + let path = cache_value.path().normalize_with(request); + let cache_value = self.cache.value(&path); // a. LOAD_AS_FILE(Y + X) if !request.ends_with('/') { - if let Some(path) = self.load_as_file(&path)? { + if let Some(path) = self.load_as_file(&cache_value)? { return Ok(path); } } // b. LOAD_AS_DIRECTORY(Y + X) - if let Some(path) = self.load_as_directory(&path)? { + if let Some(path) = self.load_as_directory(&cache_value)? { return Ok(path); } // c. THROW "not found" Err(ResolveError::NotFound(path.into_boxed_path())) } - fn require_path(&self, path: &Path, request: &str) -> Result { - let dirname = self.cache.dirname(path); + fn require_path( + &self, + cache_value: &Arc, + request: &str, + ) -> Result, ResolveError> { + let dirname = self.cache.dirname(cache_value); // 5. LOAD_PACKAGE_SELF(X, dirname(Y)) if let Some(path) = self.load_package_self(&dirname, request)? { return Ok(path); @@ -158,44 +175,48 @@ impl ResolverGeneric { if let Some(path) = self.load_node_modules(&dirname, request)? { return Ok(path); } - if let Some(path) = self.load_as_file(&path.join(request))? { + let cache_value = self.cache.value(&cache_value.path().join(request)); + if let Some(path) = self.load_as_file(&cache_value)? { return Ok(path); } // 7. THROW "not found" - Err(ResolveError::NotFound(path.to_path_buf().into_boxed_path())) + Err(ResolveError::NotFound(cache_value.to_path_buf().into_boxed_path())) } - fn load_as_file(&self, path: &Path) -> ResolveState { + fn load_as_file(&self, cache_value: &Arc) -> ResolveState { // enhanced-resolve feature: extension_alias - if let Some(path) = self.load_extension_alias(path)? { + if let Some(path) = self.load_extension_alias(cache_value)? { return Ok(Some(path)); } // 1. If X is a file, load X as its file extension format. STOP - if let Some(path) = self.load_alias_or_file(path)? { + // let cache_value = self.cache.cache_value(&path); + if let Some(path) = self.load_alias_or_file(cache_value)? { return Ok(Some(path)); } // 2. If X.js is a file, load X.js as JavaScript text. STOP // 3. If X.json is a file, parse X.json to a JavaScript Object. STOP // 4. If X.node is a file, load X.node as binary addon. STOP for extension in &self.options.extensions { - let path_with_extension = path.with_extension(extension); - if let Some(path) = self.load_alias_or_file(&path_with_extension)? { + let path_with_extension = cache_value.path().with_extension(extension); + let cache_value = self.cache.value(&path_with_extension); + if let Some(path) = self.load_alias_or_file(&cache_value)? { return Ok(Some(path)); } } Ok(None) } - fn load_symlink(&self, path: &Path) -> Option { - if self.options.symlinks { self.cache.canonicalize(path) } else { None } + fn load_symlink(&self, cache_value: &Arc) -> Option { + if self.options.symlinks { cache_value.symlink(&self.cache.fs) } else { None } } - fn load_index(&self, path: &Path) -> ResolveState { + fn load_index(&self, cache_value: &Arc) -> ResolveState { for main_field in &self.options.main_files { - let main_path = path.join(main_field); + let main_path = cache_value.path().join(main_field); if self.options.enforce_extension == Some(false) { - if let Some(path) = self.load_alias_or_file(&main_path)? { + let cache_value = self.cache.value(&main_path); + if let Some(path) = self.load_alias_or_file(&cache_value)? { return Ok(Some(path)); } } @@ -204,7 +225,8 @@ impl ResolverGeneric { // 3. If X/index.node is a file, load X/index.node as binary addon. STOP for extension in &self.options.extensions { let main_path_with_extension = main_path.with_extension(extension); - if let Some(path) = self.load_alias_or_file(&main_path_with_extension)? { + let cache_value = self.cache.value(&main_path_with_extension); + if let Some(path) = self.load_alias_or_file(&cache_value)? { return Ok(Some(path)); } } @@ -212,35 +234,37 @@ impl ResolverGeneric { Ok(None) } - fn load_alias_or_file(&self, path: &Path) -> ResolveState { - if let Some(package_json) = self.cache.find_package_json(path)? { + fn load_alias_or_file(&self, cache_value: &Arc) -> ResolveState { + if let Some(package_json) = cache_value.find_package_json(&self.cache.fs)? { + let path = cache_value.path(); if let Some(path) = self.load_browser_field(path, None, &package_json)? { return Ok(Some(path)); } } - if self.cache.is_file(path) { - return Ok(Some(path.to_path_buf())); + if cache_value.is_file(&self.cache.fs) { + return Ok(Some(Arc::clone(cache_value))); } Ok(None) } - fn load_as_directory(&self, path: &Path) -> ResolveState { + fn load_as_directory(&self, cache_value: &Arc) -> ResolveState { // TODO: Only package.json is supported, so warn about having other values // Checking for empty files is needed for omitting checks on package.json // 1. If X/package.json is a file, if !self.options.description_files.is_empty() { // a. Parse X/package.json, and look for "main" field. - if let Some(package_json) = self.cache.get_package_json(path)? { + if let Some(package_json) = cache_value.package_json(&self.cache.fs).transpose()? { // b. If "main" is a falsy value, GOTO 2. if let Some(main_field) = &package_json.main { // c. let M = X + (json main field) - let main_field_path = path.normalize_with(main_field); + let main_field_path = cache_value.path().normalize_with(main_field); // d. LOAD_AS_FILE(M) - if let Some(path) = self.load_as_file(&main_field_path)? { + let cache_value = self.cache.value(&main_field_path); + if let Some(path) = self.load_as_file(&cache_value)? { return Ok(Some(path)); } // e. LOAD_INDEX(M) - if let Some(path) = self.load_index(&main_field_path)? { + if let Some(path) = self.load_index(&cache_value)? { return Ok(Some(path)); } // f. LOAD_INDEX(X) DEPRECATED @@ -248,18 +272,18 @@ impl ResolverGeneric { return Err(ResolveError::NotFound(main_field_path.into_boxed_path())); } - if let Some(path) = self.load_index(path)? { + if let Some(path) = self.load_index(cache_value)? { return Ok(Some(path)); } } } // 2. LOAD_INDEX(X) - self.load_index(path) + self.load_index(cache_value) } - fn load_node_modules(&self, start: &Path, request: &str) -> ResolveState { + fn load_node_modules(&self, cache_value: &Arc, request: &str) -> ResolveState { // 1. let DIRS = NODE_MODULES_PATHS(START) - let dirs = self.node_module_paths(start); + let dirs = self.node_module_paths(cache_value.path()); // 2. for each DIR in DIRS: for node_module_path in dirs { // a. LOAD_PACKAGE_EXPORTS(X, DIR) @@ -268,15 +292,16 @@ impl ResolverGeneric { } let node_module_file = node_module_path.join(request); + let cache_value = self.cache.value(&node_module_file); // b. LOAD_AS_FILE(DIR/X) if !request.ends_with('/') { - if let Some(path) = self.load_as_file(&node_module_file)? { + if let Some(path) = self.load_as_file(&cache_value)? { return Ok(Some(path)); } } // c. LOAD_AS_DIRECTORY(DIR/X) - if self.cache.is_dir(&node_module_file) { - if let Some(path) = self.load_as_directory(&node_module_file)? { + if cache_value.is_dir(&self.cache.fs) { + if let Some(path) = self.load_as_directory(&cache_value)? { return Ok(Some(path)); } } @@ -306,9 +331,11 @@ impl ResolverGeneric { /// # Panics /// /// * Parent of package.json is None - fn load_package_self(&self, path: &Path, request: &str) -> ResolveState { - if let Some(package_json) = self.cache.find_package_json(path)? { - if let Some(path) = self.load_browser_field(path, Some(request), &package_json)? { + fn load_package_self(&self, cache_value: &Arc, request: &str) -> ResolveState { + if let Some(package_json) = cache_value.find_package_json(&self.cache.fs)? { + if let Some(path) = + self.load_browser_field(cache_value.path(), Some(request), &package_json)? + { return Ok(Some(path)); } } @@ -325,12 +352,18 @@ impl ResolverGeneric { let request = Request::parse(request).map_err(ResolveError::Request)?; debug_assert!(package_json.path.file_name().is_some_and(|x| x == "package.json")); // TODO: Do we need to pass query and fragment? - return self.require(package_json.path.parent().unwrap(), &request).map(Some); + let cache_value = self.cache.value(package_json.path.parent().unwrap()); + return self.require(&cache_value, &request).map(Some); } Ok(None) } - fn load_alias(&self, path: &Path, request: &str, alias: &Alias) -> ResolveState { + fn load_alias( + &self, + cache_value: &Arc, + request: &str, + alias: &Alias, + ) -> ResolveState { for (alias, requests) in alias { let exact_match = alias.strip_prefix(request).is_some_and(|c| c == "$"); if request.starts_with(alias) || exact_match { @@ -344,14 +377,16 @@ impl ResolverGeneric { }; let new_request = Request::parse(&new_request).map_err(ResolveError::Request)?; - match self.require(path, &new_request) { + match self.require(cache_value, &new_request) { Err(ResolveError::NotFound(_)) => { /* noop */ } Ok(path) => return Ok(Some(path)), Err(err) => return Err(err), } } AliasValue::Ignore => { - return Err(ResolveError::Ignored(path.join(alias).into_boxed_path())); + return Err(ResolveError::Ignored( + cache_value.path().join(alias).into_boxed_path(), + )); } } } @@ -369,32 +404,39 @@ impl ResolverGeneric { /// # Errors /// /// * [ResolveError::ExtensionAlias]: When all of the aliased extensions are not found - fn load_extension_alias(&self, path: &Path) -> ResolveState { - let Some(path_extension) = path.extension() else { return Ok(None) }; + fn load_extension_alias(&self, cache_value: &Arc) -> ResolveState { + let Some(path_extension) = cache_value.path().extension() else { return Ok(None) }; let Some((_, extensions)) = self.options.extension_alias.iter().find(|(ext, _)| OsStr::new(ext) == path_extension) else { return Ok(None); }; for extension in extensions { - let path_with_extension = path.with_extension(extension); - if self.cache.is_file(&path_with_extension) { - return Ok(Some(path_with_extension)); + let path_with_extension = cache_value.path().with_extension(extension); + let cache_value = self.cache.value(&path_with_extension); + if cache_value.is_file(&self.cache.fs) { + return Ok(Some(cache_value)); } } Err(ResolveError::ExtensionAlias) } - fn load_roots(&self, path: &Path, request: &str) -> Result { + fn load_roots( + &self, + cache_value: &Arc, + request: &str, + ) -> Result, ResolveError> { debug_assert!(request.starts_with('/')); if self.options.roots.is_empty() { - return self.require_path(Path::new("/"), request); + let cache_value = self.cache.value(Path::new("/")); + return self.require_path(&cache_value, request); } for root in &self.options.roots { - if let Ok(path) = self.require_relative(root, request.trim_start_matches('/')) { + let cache_value = self.cache.value(root); + if let Ok(path) = self.require_relative(&cache_value, request.trim_start_matches('/')) { return Ok(path); } } - Err(ResolveError::NotFound(path.to_path_buf().into_boxed_path())) + Err(ResolveError::NotFound(cache_value.to_path_buf().into_boxed_path())) } } diff --git a/crates/oxc_resolver/tests/enhanced_resolve/test/roots.rs b/crates/oxc_resolver/tests/enhanced_resolve/test/roots.rs index 599214f10..408965ce7 100644 --- a/crates/oxc_resolver/tests/enhanced_resolve/test/roots.rs +++ b/crates/oxc_resolver/tests/enhanced_resolve/test/roots.rs @@ -31,7 +31,7 @@ fn roots() { #[rustfmt::skip] let fail = [ - ("should not work with relative path", "fixtures/b.js", ResolveError::NotFound(f.clone().into_boxed_path())) + ("should not work with relative path", "fixtures/b.js", ResolveError::NotFound(f.join("fixtures/b.js").into_boxed_path())) ]; for (comment, request, expected) in fail {