diff --git a/apps/oxlint/src/js_plugins/external_linter.rs b/apps/oxlint/src/js_plugins/external_linter.rs index 670552ee3ad81..9e735efd899fe 100644 --- a/apps/oxlint/src/js_plugins/external_linter.rs +++ b/apps/oxlint/src/js_plugins/external_linter.rs @@ -77,12 +77,11 @@ fn wrap_lint_file(cb: JsLintFileCb) -> ExternalLinterLintFileCb { let (tx, rx) = channel(); // SAFETY: This function is only called when an `ExternalLinter` exists. - // When that is the case, the `AllocatorPool` used to create `Allocator`s is created with - // `AllocatorPool::new_fixed_size`, so all `Allocator`s are created via `FixedSizeAllocator`. - // This is somewhat sketchy, as we don't have a type-level guarantee of this invariant, - // but it does hold at present. - // When we replace `bumpalo` with a custom allocator, we can close this soundness hole. - // TODO: Do that. + // When that is the case, the `AllocatorPool` is created with `AllocatorPool::new_dual`, + // and `Allocator`s for files that need linting are created via `FixedSizeAllocator` + // using `get_maybe_fixed_size(true)`. + // This is validated with debug assertions in the linter runtime before calling this function. + // The `AllocatorGuard` tracks whether an allocator is fixed-size via the `is_fixed_size` field. let (buffer_id, buffer) = unsafe { get_buffer(allocator) }; // Send data to JS diff --git a/apps/oxlint/src/js_plugins/raw_fs.rs b/apps/oxlint/src/js_plugins/raw_fs.rs index e19887a811809..32bef88aff291 100644 --- a/apps/oxlint/src/js_plugins/raw_fs.rs +++ b/apps/oxlint/src/js_plugins/raw_fs.rs @@ -6,7 +6,7 @@ use std::{ ptr, slice, }; -use oxc_allocator::Allocator; +use oxc_allocator::{Allocator, AllocatorGuard}; use oxc_linter::RuntimeFileSystem; /// File system used when JS plugins are in use. @@ -14,8 +14,9 @@ use oxc_linter::RuntimeFileSystem; /// Identical to `OsFileSystem`, except that `read_to_arena_str` reads the file's contents into /// start of the allocator, instead of the end. This conforms to what raw transfer needs. /// -/// Must only be used in conjunction with `AllocatorPool` created with `new_fixed_size`, -/// which wraps `Allocator`s with a custom `Drop` impl, which makes `read_to_arena_str` safe. +/// Must only be used with fixed-size `Allocator`s (created via `AllocatorPool::new_fixed_size` +/// or `AllocatorPool::new_dual` with `get_maybe_fixed_size(true)`), +/// which have a custom `Drop` impl that makes `read_to_arena_str` safe. /// /// This is a temporary solution. When we replace `bumpalo` with our own allocator, all strings /// will be written at start of the arena, so then `OsFileSystem` will work fine, and we can @@ -33,10 +34,15 @@ impl RuntimeFileSystem for RawTransferFileSystem { fn read_to_arena_str<'a>( &self, path: &Path, - allocator: &'a Allocator, + allocator_guard: &'a AllocatorGuard, ) -> Result<&'a str, std::io::Error> { - // SAFETY: Caller promises not to allow `allocator` to be dropped - unsafe { read_to_arena_str(path, allocator) } + #[cfg(all(feature = "napi", target_pointer_width = "64", target_endian = "little"))] + if allocator_guard.is_fixed_size() { + // SAFETY: Caller promises not to allow `allocator` to be dropped + unsafe { return read_to_arena_str(path, allocator_guard) } + } + + oxc_linter::read_to_arena_str(path, allocator_guard) } fn write_file(&self, path: &Path, content: &str) -> Result<(), std::io::Error> { diff --git a/crates/oxc_allocator/src/pool/mod.rs b/crates/oxc_allocator/src/pool/mod.rs index 5c558da03de39..3b4b06cb00f78 100644 --- a/crates/oxc_allocator/src/pool/mod.rs +++ b/crates/oxc_allocator/src/pool/mod.rs @@ -18,39 +18,70 @@ pub use fixed_size::{FixedSizeAllocatorMetadata, free_fixed_size_allocator}; /// A thread-safe pool for reusing [`Allocator`] instances to reduce allocation overhead. /// -/// Uses either: -/// 1. Standard allocators - suitable for general use. -/// 2. Fixed-size allocators - compatible with raw transfer. +/// Supports three configurations: +/// 1. Standard allocators only - suitable for general use ([`AllocatorPool::new`]). +/// 2. Fixed-size allocators only - compatible with raw transfer ([`AllocatorPool::new_fixed_size`]). +/// 3. Dual mode with both types - allows choosing allocator type at runtime ([`AllocatorPool::new_dual`]). /// -/// Standard allocator pool is created by [`AllocatorPool::new`]. -/// Fixed-size allocator pool is created by [`AllocatorPool::new_fixed_size`]. +/// The dual mode is useful when some allocations need raw transfer (e.g., files to be linted with JS plugins) +/// while others don't (e.g., dependency files only parsed for module resolution). /// /// Fixed-size allocators are only supported on 64-bit little-endian platforms at present, /// and require the `fixed_size` Cargo feature to be enabled. -#[repr(transparent)] -pub struct AllocatorPool(AllocatorPoolInner); - -/// Inner type of [`AllocatorPool`], holding either a standard or fixed-size allocator pool. -enum AllocatorPoolInner { - Standard(StandardAllocatorPool), +pub struct AllocatorPool { + standard: StandardAllocatorPool, #[cfg(all(feature = "fixed_size", target_pointer_width = "64", target_endian = "little"))] - FixedSize(FixedSizeAllocatorPool), + fixed_size: FixedSizeAllocatorPool, } impl AllocatorPool { /// Create a new [`AllocatorPool`] for use across the specified number of threads, - /// which uses standard allocators. + /// which uses standard allocators only. pub fn new(thread_count: usize) -> AllocatorPool { - Self(AllocatorPoolInner::Standard(StandardAllocatorPool::new(thread_count))) + Self { + standard: StandardAllocatorPool::new(thread_count), + #[cfg(all( + feature = "fixed_size", + target_pointer_width = "64", + target_endian = "little" + ))] + fixed_size: FixedSizeAllocatorPool::new(0), + } } /// Create a new [`AllocatorPool`] for use across the specified number of threads, - /// which uses fixed-size allocators (suitable for raw transfer). + /// which uses fixed-size allocators only (suitable for raw transfer). #[cfg(feature = "fixed_size")] pub fn new_fixed_size(thread_count: usize) -> AllocatorPool { #[cfg(all(target_pointer_width = "64", target_endian = "little"))] { - Self(AllocatorPoolInner::FixedSize(FixedSizeAllocatorPool::new(thread_count))) + Self { + standard: StandardAllocatorPool::new(0), // Not used, but need to initialize + fixed_size: FixedSizeAllocatorPool::new(thread_count), + } + } + + #[cfg(not(all(target_pointer_width = "64", target_endian = "little")))] + { + let _thread_count = thread_count; // Avoid unused vars lint warning + panic!("Fixed size allocators are only supported on 64-bit little-endian platforms"); + } + } + + /// Create a new [`AllocatorPool`] for use across the specified number of threads, + /// which contains both standard and fixed-size allocators. + /// + /// This allows choosing the allocator type at runtime via [`AllocatorPool::get_maybe_fixed_size`]. + /// Useful when some allocations need raw transfer (e.g., files to be linted with JS plugins) + /// while others don't (e.g., dependency files only parsed for module resolution). + #[cfg(feature = "fixed_size")] + pub fn new_dual(thread_count: usize) -> AllocatorPool { + #[cfg(all(target_pointer_width = "64", target_endian = "little"))] + { + Self { + standard: StandardAllocatorPool::new(thread_count), + fixed_size: FixedSizeAllocatorPool::new(thread_count), + } } #[cfg(not(all(target_pointer_width = "64", target_endian = "little")))] @@ -64,21 +95,80 @@ impl AllocatorPool { /// /// Returns an [`AllocatorGuard`] that gives access to the allocator. /// + /// If the pool was created with [`AllocatorPool::new_fixed_size`], returns a fixed-size allocator. + /// Otherwise, returns a standard allocator. + /// /// # Panics /// /// Panics if the underlying mutex is poisoned. pub fn get(&self) -> AllocatorGuard<'_> { - let allocator = match &self.0 { - AllocatorPoolInner::Standard(pool) => pool.get(), + if cfg!(all(feature = "fixed_size", target_pointer_width = "64", target_endian = "little")) + { + let allocator = self.fixed_size.get(); + return AllocatorGuard { + allocator: ManuallyDrop::new(allocator), + is_fixed_size: true, + pool: self, + }; + } + + let allocator = self.standard.get(); + AllocatorGuard { + allocator: ManuallyDrop::new(allocator), #[cfg(all( feature = "fixed_size", target_pointer_width = "64", target_endian = "little" ))] - AllocatorPoolInner::FixedSize(pool) => pool.get(), - }; + is_fixed_size: false, + pool: self, + } + } - AllocatorGuard { allocator: ManuallyDrop::new(allocator), pool: self } + /// Retrieve an [`Allocator`] from the pool, choosing between standard and fixed-size allocators. + /// + /// Returns an [`AllocatorGuard`] that gives access to the allocator. + /// + /// # Parameters + /// + /// * `fixed_size` - If `true`, returns a fixed-size allocator (suitable for raw transfer). + /// If `false`, returns a standard allocator. + /// + /// # Panics + /// + /// Panics if: + /// - The underlying mutex is poisoned. + /// - `fixed_size` is `true` but the pool was not created with [`AllocatorPool::new_dual`] or [`AllocatorPool::new_fixed_size`]. + #[cfg(feature = "fixed_size")] + pub fn get_maybe_fixed_size(&self, fixed_size: bool) -> AllocatorGuard<'_> { + #[cfg(all(target_pointer_width = "64", target_endian = "little"))] + { + if fixed_size { + let allocator = self.fixed_size.get(); + return AllocatorGuard { + allocator: ManuallyDrop::new(allocator), + is_fixed_size: true, + pool: self, + }; + } + } + + #[cfg(not(all(target_pointer_width = "64", target_endian = "little")))] + if fixed_size { + panic!("Fixed size allocators are only supported on 64-bit little-endian platforms"); + } + + let allocator = self.standard.get(); + AllocatorGuard { + allocator: ManuallyDrop::new(allocator), + #[cfg(all( + feature = "fixed_size", + target_pointer_width = "64", + target_endian = "little" + ))] + is_fixed_size: false, + pool: self, + } } /// Add an [`Allocator`] to the pool. @@ -88,20 +178,32 @@ impl AllocatorPool { /// # Panics /// /// Panics if the underlying mutex is poisoned. + #[cfg(all(feature = "fixed_size", target_pointer_width = "64", target_endian = "little"))] + fn add(&self, allocator: Allocator, is_fixed_size: bool) { + // SAFETY: This method is only called from `AllocatorGuard::drop`. + // `AllocatorGuard`s are only created by `AllocatorPool::get` or `AllocatorPool::get_maybe_fixed_size`, + // so the `Allocator` must have been created by this pool. Therefore, it is the correct type for the pool. + unsafe { + if is_fixed_size { + self.fixed_size.add(allocator); + } else { + self.standard.add(allocator); + } + } + } + + /// Add an [`Allocator`] to the pool (for platforms without fixed-size support). + #[cfg(not(all( + feature = "fixed_size", + target_pointer_width = "64", + target_endian = "little" + )))] fn add(&self, allocator: Allocator) { // SAFETY: This method is only called from `AllocatorGuard::drop`. // `AllocatorGuard`s are only created by `AllocatorPool::get`, so the `Allocator` must have // been created by this pool. Therefore, it is the correct type for the pool. unsafe { - match &self.0 { - AllocatorPoolInner::Standard(pool) => pool.add(allocator), - #[cfg(all( - feature = "fixed_size", - target_pointer_width = "64", - target_endian = "little" - ))] - AllocatorPoolInner::FixedSize(pool) => pool.add(allocator), - } + self.standard.add(allocator); } } } @@ -111,9 +213,19 @@ impl AllocatorPool { /// On drop, the `Allocator` is reset and returned to the pool. pub struct AllocatorGuard<'alloc_pool> { allocator: ManuallyDrop, + #[cfg(all(feature = "fixed_size", target_pointer_width = "64", target_endian = "little"))] + is_fixed_size: bool, pool: &'alloc_pool AllocatorPool, } +impl AllocatorGuard<'_> { + /// Returns `true` if this allocator is a fixed-size allocator (suitable for raw transfer). + #[cfg(all(feature = "fixed_size", target_pointer_width = "64", target_endian = "little"))] + pub fn is_fixed_size(&self) -> bool { + self.is_fixed_size + } +} + impl Deref for AllocatorGuard<'_> { type Target = Allocator; @@ -127,6 +239,15 @@ impl Drop for AllocatorGuard<'_> { fn drop(&mut self) { // SAFETY: After taking ownership of the `Allocator`, we do not touch the `ManuallyDrop` again let allocator = unsafe { ManuallyDrop::take(&mut self.allocator) }; + + #[cfg(all(feature = "fixed_size", target_pointer_width = "64", target_endian = "little"))] + self.pool.add(allocator, self.is_fixed_size); + + #[cfg(not(all( + feature = "fixed_size", + target_pointer_width = "64", + target_endian = "little" + )))] self.pool.add(allocator); } } diff --git a/crates/oxc_language_server/src/linter/isolated_lint_handler.rs b/crates/oxc_language_server/src/linter/isolated_lint_handler.rs index 0a1f91515ef6a..f1d4edfab34ba 100644 --- a/crates/oxc_language_server/src/linter/isolated_lint_handler.rs +++ b/crates/oxc_language_server/src/linter/isolated_lint_handler.rs @@ -8,7 +8,6 @@ use oxc_data_structures::rope::Rope; use rustc_hash::FxHashSet; use tower_lsp_server::{UriExt, lsp_types::Uri}; -use oxc_allocator::Allocator; use oxc_linter::{ AllowWarnDeny, ConfigStore, DisableDirectives, Fix, FixKind, LINTABLE_EXTENSIONS, LintOptions, LintRunner, LintRunnerBuilder, LintServiceOptions, Linter, Message, PossibleFixes, @@ -49,13 +48,13 @@ impl RuntimeFileSystem for IsolatedLintHandlerFileSystem { fn read_to_arena_str<'a>( &'a self, path: &Path, - allocator: &'a Allocator, + allocator_guard: &'a oxc_allocator::AllocatorGuard, ) -> Result<&'a str, std::io::Error> { if path == self.path_to_lint { return Ok(&self.source_text); } - read_to_arena_str(path, allocator) + read_to_arena_str(path, allocator_guard) } fn write_file(&self, _path: &Path, _content: &str) -> Result<(), std::io::Error> { diff --git a/crates/oxc_linter/src/service/runtime.rs b/crates/oxc_linter/src/service/runtime.rs index 8946ff9065035..e22d381703a37 100644 --- a/crates/oxc_linter/src/service/runtime.rs +++ b/crates/oxc_linter/src/service/runtime.rs @@ -209,10 +209,11 @@ impl Runtime { let thread_count = rayon::current_num_threads(); - // If an external linter is used (JS plugins), we must use fixed-size allocators, - // for compatibility with raw transfer + // If an external linter is used (JS plugins), we use dual allocators: + // - Fixed-size allocators for files that need linting (compatible with raw transfer to JS) + // - Standard allocators for dependency files (only parsed for module resolution) let allocator_pool = if linter.has_external_linter() { - AllocatorPool::new_fixed_size(thread_count) + AllocatorPool::new_dual(thread_count) } else { AllocatorPool::new(thread_count) }; @@ -598,6 +599,13 @@ impl Runtime { return; } + if me.linter.has_external_linter() { + debug_assert!( + allocator_guard.is_fixed_size(), + "External linters require fixed-size allocators for raw transfer" + ); + } + let (mut messages, disable_directives) = me .linter .run_with_disable_directives(path, context_sub_hosts, allocator_guard); @@ -712,6 +720,15 @@ impl Runtime { } let path = Path::new(&module_to_lint.path); + + // Validate that fixed-size allocator is used when external linter is present + if me.linter.has_external_linter() { + debug_assert!( + allocator_guard.is_fixed_size(), + "External linters require fixed-size allocators for raw transfer" + ); + } + let (section_messages, disable_directives) = me .linter .run_with_disable_directives(path, context_sub_hosts, allocator_guard); @@ -785,6 +802,13 @@ impl Runtime { return; } + if me.linter.has_external_linter() { + debug_assert!( + allocator_guard.is_fixed_size(), + "External linters require fixed-size allocators for raw transfer" + ); + } + messages.lock().unwrap().extend( me.linter.run( Path::new(&module.path), @@ -831,9 +855,18 @@ impl Runtime { return None; } - let allocator_guard = self.allocator_pool.get(); + // Choose allocator type based on whether this file needs linting with JS plugins: + // - Files in `paths` set are entry files that need linting + // - Files not in `paths` set are dependency files (only parsed for module resolution) + let file_needs_linting = paths.contains(path); + + let allocator_guard = { + let file_needs_linting_with_js_plugins = + file_needs_linting && self.linter.has_external_linter(); + self.allocator_pool.get_maybe_fixed_size(file_needs_linting_with_js_plugins) + }; - if paths.contains(path) { + if file_needs_linting { let mut records = SmallVec::<[Result>; 1]>::new(); diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs index d7fe212318b31..d56dd902160e3 100644 --- a/crates/oxc_linter/src/tester.rs +++ b/crates/oxc_linter/src/tester.rs @@ -202,12 +202,12 @@ impl RuntimeFileSystem for TesterFileSystem { fn read_to_arena_str<'a>( &self, path: &Path, - allocator: &'a Allocator, + allocator_guard: &'a oxc_allocator::AllocatorGuard, ) -> Result<&'a str, std::io::Error> { if path == self.path_to_lint { - return Ok(allocator.alloc_str(&self.source_text)); + return Ok(allocator_guard.alloc_str(&self.source_text)); } - read_to_arena_str(path, allocator) + read_to_arena_str(path, allocator_guard) } fn write_file(&self, _path: &Path, _content: &str) -> Result<(), std::io::Error> {