From c67dfaec61fa32800bb714178bdb6c03674b7d71 Mon Sep 17 00:00:00 2001 From: Gustrb Date: Fri, 9 Jun 2023 15:41:18 -0300 Subject: [PATCH] perf(node): cache realpath_sync calls in read permission check (#19379) --- cli/npm/resolvers/common.rs | 94 +++++++++++++++++++++++++------------ cli/npm/resolvers/global.rs | 18 ++++--- cli/npm/resolvers/local.rs | 20 ++++---- 3 files changed, 87 insertions(+), 45 deletions(-) diff --git a/cli/npm/resolvers/common.rs b/cli/npm/resolvers/common.rs index b352229c19..e0705f12bb 100644 --- a/cli/npm/resolvers/common.rs +++ b/cli/npm/resolvers/common.rs @@ -1,9 +1,11 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. +use std::collections::HashMap; use std::io::ErrorKind; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use std::sync::Mutex; use async_trait::async_trait; use deno_ast::ModuleSpecifier; @@ -54,6 +56,69 @@ pub trait NpmPackageFsResolver: Send + Sync { ) -> Result<(), AnyError>; } +#[derive(Debug)] +pub struct RegistryReadPermissionChecker { + fs: Arc, + cache: Mutex>, + registry_path: PathBuf, +} + +impl RegistryReadPermissionChecker { + pub fn new(fs: Arc, registry_path: PathBuf) -> Self { + Self { + fs, + registry_path, + cache: Default::default(), + } + } + + pub fn ensure_registry_read_permission( + &self, + permissions: &dyn NodePermissions, + path: &Path, + ) -> Result<(), AnyError> { + // allow reading if it's in the node_modules + let is_path_in_node_modules = path.starts_with(&self.registry_path) + && path + .components() + .all(|c| !matches!(c, std::path::Component::ParentDir)); + + if is_path_in_node_modules { + let mut cache = self.cache.lock().unwrap(); + let registry_path_canon = match cache.get(&self.registry_path) { + Some(canon) => canon.clone(), + None => { + let canon = self.fs.realpath_sync(&self.registry_path)?; + cache.insert(self.registry_path.to_path_buf(), canon.clone()); + canon + } + }; + + let path_canon = match cache.get(path) { + Some(canon) => canon.clone(), + None => { + let canon = self.fs.realpath_sync(path); + if let Err(e) = &canon { + if e.kind() == ErrorKind::NotFound { + return Ok(()); + } + } + + let canon = canon?; + cache.insert(path.to_path_buf(), canon.clone()); + canon + } + }; + + if path_canon.starts_with(registry_path_canon) { + return Ok(()); + } + } + + permissions.check_read(path) + } +} + /// Caches all the packages in parallel. pub async fn cache_packages( mut packages: Vec, @@ -90,35 +155,6 @@ pub async fn cache_packages( Ok(()) } -pub fn ensure_registry_read_permission( - fs: &Arc, - permissions: &dyn NodePermissions, - registry_path: &Path, - path: &Path, -) -> Result<(), AnyError> { - // allow reading if it's in the node_modules - if path.starts_with(registry_path) - && path - .components() - .all(|c| !matches!(c, std::path::Component::ParentDir)) - { - // todo(dsherret): cache this? - if let Ok(registry_path) = fs.realpath_sync(registry_path) { - match fs.realpath_sync(path) { - Ok(path) if path.starts_with(registry_path) => { - return Ok(()); - } - Err(e) if e.kind() == ErrorKind::NotFound => { - return Ok(()); - } - _ => {} // ignore - } - } - } - - permissions.check_read(path) -} - /// Gets the corresponding @types package for the provided package name. pub fn types_package_name(package_name: &str) -> String { debug_assert!(!package_name.starts_with("@types/")); diff --git a/cli/npm/resolvers/global.rs b/cli/npm/resolvers/global.rs index 10a17f6900..ca84d7e8bc 100644 --- a/cli/npm/resolvers/global.rs +++ b/cli/npm/resolvers/global.rs @@ -23,18 +23,18 @@ use crate::npm::resolution::NpmResolution; use crate::npm::resolvers::common::cache_packages; use crate::npm::NpmCache; -use super::common::ensure_registry_read_permission; use super::common::types_package_name; use super::common::NpmPackageFsResolver; +use super::common::RegistryReadPermissionChecker; /// Resolves packages from the global npm cache. #[derive(Debug)] pub struct GlobalNpmPackageResolver { - fs: Arc, cache: Arc, resolution: Arc, registry_url: Url, system_info: NpmSystemInfo, + registry_read_permission_checker: RegistryReadPermissionChecker, } impl GlobalNpmPackageResolver { @@ -46,11 +46,14 @@ impl GlobalNpmPackageResolver { system_info: NpmSystemInfo, ) -> Self { Self { - fs, - cache, + cache: cache.clone(), resolution, - registry_url, + registry_url: registry_url.clone(), system_info, + registry_read_permission_checker: RegistryReadPermissionChecker::new( + fs, + cache.registry_folder(®istry_url), + ), } } @@ -156,7 +159,8 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver { permissions: &dyn NodePermissions, path: &Path, ) -> Result<(), AnyError> { - let registry_path = self.cache.registry_folder(&self.registry_url); - ensure_registry_read_permission(&self.fs, permissions, ®istry_path, path) + self + .registry_read_permission_checker + .ensure_registry_read_permission(permissions, path) } } diff --git a/cli/npm/resolvers/local.rs b/cli/npm/resolvers/local.rs index a708f86914..976038404d 100644 --- a/cli/npm/resolvers/local.rs +++ b/cli/npm/resolvers/local.rs @@ -41,9 +41,9 @@ use crate::npm::NpmCache; use crate::util::fs::copy_dir_recursive; use crate::util::fs::hard_link_dir_recursive; -use super::common::ensure_registry_read_permission; use super::common::types_package_name; use super::common::NpmPackageFsResolver; +use super::common::RegistryReadPermissionChecker; /// Resolver that creates a local node_modules directory /// and resolves packages from it. @@ -57,6 +57,7 @@ pub struct LocalNpmPackageResolver { root_node_modules_path: PathBuf, root_node_modules_url: Url, system_info: NpmSystemInfo, + registry_read_permission_checker: RegistryReadPermissionChecker, } impl LocalNpmPackageResolver { @@ -70,15 +71,19 @@ impl LocalNpmPackageResolver { system_info: NpmSystemInfo, ) -> Self { Self { - fs, + fs: fs.clone(), cache, progress_bar, resolution, registry_url, root_node_modules_url: Url::from_directory_path(&node_modules_folder) .unwrap(), - root_node_modules_path: node_modules_folder, + root_node_modules_path: node_modules_folder.clone(), system_info, + registry_read_permission_checker: RegistryReadPermissionChecker::new( + fs, + node_modules_folder, + ), } } @@ -227,12 +232,9 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver { permissions: &dyn NodePermissions, path: &Path, ) -> Result<(), AnyError> { - ensure_registry_read_permission( - &self.fs, - permissions, - &self.root_node_modules_path, - path, - ) + self + .registry_read_permission_checker + .ensure_registry_read_permission(permissions, path) } }