From 95bc0d7d96dca66a3712eb24618a63f41f0d1f40 Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Sat, 18 Jan 2025 01:23:55 +0000 Subject: [PATCH] refactor(allocator)!: `Allocator` do not deref to `bumpalo::Bump` (#8569) Do not expose that our arena allocator is based on Bumpalo, by removing `Deref` and `DerefMut` impls. Instead implement the methods we use on `Allocator` itself. That our allocator is based on Bumpalo is now an internal implementation detail. This will allow us to replace the allocator in future, and enables statically preventing `Drop` types from being stored in the arena (next PR in this stack). I've intentionally only implemented a small subset of `Bump`'s methods - only `alloc`, `alloc_str`, and `reset`. This will make it simpler to implement a new allocator in future, without having to cover all of bumpalo's large API surface. In the meantime, if it turns out we need additional methods, it will be simple to add them, by just delegating to that method on `Bump`. --- crates/oxc_allocator/src/allocator_api2.rs | 23 ++-- crates/oxc_allocator/src/hash_map.rs | 5 +- crates/oxc_allocator/src/lib.rs | 132 ++++++++++++++++----- crates/oxc_allocator/src/string.rs | 8 +- crates/oxc_allocator/src/vec.rs | 8 +- 5 files changed, 126 insertions(+), 50 deletions(-) diff --git a/crates/oxc_allocator/src/allocator_api2.rs b/crates/oxc_allocator/src/allocator_api2.rs index cff3271ba0884..71d41448b8da2 100644 --- a/crates/oxc_allocator/src/allocator_api2.rs +++ b/crates/oxc_allocator/src/allocator_api2.rs @@ -1,3 +1,6 @@ +// All methods just delegate to `bumpalo`, so all marked `#[inline(always)]` +#![expect(clippy::inline_always)] + use std::{alloc::Layout, ptr::NonNull}; use allocator_api2::alloc::{AllocError, Allocator}; @@ -5,43 +8,43 @@ use allocator_api2::alloc::{AllocError, Allocator}; /// SAFETY: /// unsafe impl Allocator for &crate::Allocator { - #[inline] + #[inline(always)] fn allocate(&self, layout: Layout) -> Result, AllocError> { - (&self.bump).allocate(layout) + self.bump().allocate(layout) } - #[inline] + #[inline(always)] unsafe fn deallocate(&self, ptr: NonNull, layout: Layout) { - (&self.bump).deallocate(ptr, layout); + self.bump().deallocate(ptr, layout); } - #[inline] + #[inline(always)] unsafe fn shrink( &self, ptr: NonNull, old_layout: Layout, new_layout: Layout, ) -> Result, AllocError> { - (&self.bump).shrink(ptr, old_layout, new_layout) + self.bump().shrink(ptr, old_layout, new_layout) } - #[inline] + #[inline(always)] unsafe fn grow( &self, ptr: NonNull, old_layout: Layout, new_layout: Layout, ) -> Result, AllocError> { - (&self.bump).grow(ptr, old_layout, new_layout) + self.bump().grow(ptr, old_layout, new_layout) } - #[inline] + #[inline(always)] unsafe fn grow_zeroed( &self, ptr: NonNull, old_layout: Layout, new_layout: Layout, ) -> Result, AllocError> { - (&self.bump).grow_zeroed(ptr, old_layout, new_layout) + self.bump().grow_zeroed(ptr, old_layout, new_layout) } } diff --git a/crates/oxc_allocator/src/hash_map.rs b/crates/oxc_allocator/src/hash_map.rs index 153a29e623d1a..cded3efcd9ada 100644 --- a/crates/oxc_allocator/src/hash_map.rs +++ b/crates/oxc_allocator/src/hash_map.rs @@ -56,7 +56,7 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> { /// until it is first inserted into. #[inline(always)] pub fn new_in(allocator: &'alloc Allocator) -> Self { - let inner = FxHashMap::with_hasher_in(FxBuildHasher, allocator); + let inner = FxHashMap::with_hasher_in(FxBuildHasher, allocator.bump()); Self(ManuallyDrop::new(inner)) } @@ -66,7 +66,8 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> { /// If capacity is 0, the hash map will not allocate. #[inline(always)] pub fn with_capacity_in(capacity: usize, allocator: &'alloc Allocator) -> Self { - let inner = FxHashMap::with_capacity_and_hasher_in(capacity, FxBuildHasher, allocator); + let inner = + FxHashMap::with_capacity_and_hasher_in(capacity, FxBuildHasher, allocator.bump()); Self(ManuallyDrop::new(inner)) } diff --git a/crates/oxc_allocator/src/lib.rs b/crates/oxc_allocator/src/lib.rs index a0893175dec03..f446c3f4c6ac6 100644 --- a/crates/oxc_allocator/src/lib.rs +++ b/crates/oxc_allocator/src/lib.rs @@ -31,18 +31,15 @@ //! Consumers of the [`oxc` umbrella crate](https://crates.io/crates/oxc) pass //! [`Allocator`] references to other tools. //! -//! ``` +//! ```ignore //! use oxc::{allocator::Allocator, parser::Parser, span::SourceType}; //! -//! let allocator = Allocator::default() +//! let allocator = Allocator::default(); //! let parsed = Parser::new(&allocator, "let x = 1;", SourceType::default()); //! assert!(parsed.errors.is_empty()); //! ``` + #![warn(missing_docs)] -use std::{ - convert::From, - ops::{Deref, DerefMut}, -}; use bumpalo::Bump; @@ -75,46 +72,121 @@ pub struct Allocator { bump: Bump, } -/// SAFETY: Not actually safe, but for enabling `Send` for downstream crates. -unsafe impl Send for Allocator {} -/// SAFETY: Not actually safe, but for enabling `Sync` for downstream crates. -unsafe impl Sync for Allocator {} +impl Allocator { + /// Allocate an object in this [`Allocator`] and return an exclusive reference to it. + /// + /// # Panics + /// Panics if reserving space for `T` fails. + /// + /// # Example + /// ``` + /// use oxc_allocator::Allocator; + /// + /// let allocator = Allocator::default(); + /// let x = allocator.alloc([1u8; 20]); + /// assert_eq!(x, &[1u8; 20]); + /// ``` + // + // `#[inline(always)]` because this is a very hot path and `Bump::alloc` is a very small function. + // We always want it to be inlined. + #[expect(clippy::inline_always)] + #[inline(always)] + pub fn alloc(&self, val: T) -> &mut T { + self.bump.alloc(val) + } -impl From for Allocator { - fn from(bump: Bump) -> Self { - Self { bump } + /// Copy a string slice into this [`Allocator`] and return a reference to it. + /// + /// # Panics + /// Panics if reserving space for the string fails. + /// + /// # Example + /// ``` + /// use oxc_allocator::Allocator; + /// let allocator = Allocator::default(); + /// let hello = allocator.alloc_str("hello world"); + /// assert_eq!(hello, "hello world"); + /// ``` + // + // `#[inline(always)]` because this is a hot path and `Bump::alloc_str` is a very small function. + // We always want it to be inlined. + #[expect(clippy::inline_always)] + #[inline(always)] + pub fn alloc_str<'alloc>(&'alloc self, src: &str) -> &'alloc mut str { + self.bump.alloc_str(src) } -} -impl Deref for Allocator { - type Target = Bump; + /// Reset this allocator. + /// + /// Performs mass deallocation on everything allocated in this arena by resetting the pointer + /// into the underlying chunk of memory to the start of the chunk. + /// Does not run any `Drop` implementations on deallocated objects. + /// + /// If this arena has allocated multiple chunks to bump allocate into, then the excess chunks + /// are returned to the global allocator. + /// + /// ## Example + /// + /// ``` + /// use oxc_allocator::Allocator; + /// + /// let mut allocator = Allocator::default(); + /// + /// // Allocate a bunch of things. + /// { + /// for i in 0..100 { + /// allocator.alloc(i); + /// } + /// } + /// + /// // Reset the arena. + /// allocator.reset(); + /// + /// // Allocate some new things in the space previously occupied by the + /// // original things. + /// for j in 200..400 { + /// allocator.alloc(j); + /// } + /// ``` + // + // `#[inline(always)]` because it just delegates to `bumpalo` + #[expect(clippy::inline_always)] + #[inline(always)] + pub fn reset(&mut self) { + self.bump.reset(); + } - fn deref(&self) -> &Self::Target { + /// Get inner [`bumpalo::Bump`]. + /// + /// This method is not public. We don't want to expose `bumpalo::Allocator` to user. + /// The fact that we're using `bumpalo` is an internal implementation detail. + // + // `#[inline(always)]` because it's a no-op + #[expect(clippy::inline_always)] + #[inline(always)] + pub(crate) fn bump(&self) -> &Bump { &self.bump } } -impl DerefMut for Allocator { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.bump - } -} +/// SAFETY: Not actually safe, but for enabling `Send` for downstream crates. +unsafe impl Send for Allocator {} +/// SAFETY: Not actually safe, but for enabling `Sync` for downstream crates. +unsafe impl Sync for Allocator {} #[cfg(test)] mod test { - use std::ops::Deref; - - use bumpalo::Bump; - use crate::Allocator; #[test] fn test_api() { - let bump = Bump::new(); - let allocator: Allocator = bump.into(); - #[allow(clippy::explicit_deref_methods)] + let mut allocator = Allocator::default(); { - _ = allocator.deref(); + let array = allocator.alloc([123; 10]); + assert_eq!(array, &[123; 10]); + let str = allocator.alloc_str("hello"); + assert_eq!(str, "hello"); } + allocator.reset(); } } diff --git a/crates/oxc_allocator/src/string.rs b/crates/oxc_allocator/src/string.rs index e87a4f1d148df..90c7d8899249d 100644 --- a/crates/oxc_allocator/src/string.rs +++ b/crates/oxc_allocator/src/string.rs @@ -38,7 +38,7 @@ impl<'alloc> String<'alloc> { /// [`with_capacity_in`]: String::with_capacity_in #[inline(always)] pub fn new_in(allocator: &'alloc Allocator) -> String<'alloc> { - Self(BumpaloString::new_in(allocator)) + Self(BumpaloString::new_in(allocator.bump())) } /// Creates a new empty [`String`] with specified capacity. @@ -57,7 +57,7 @@ impl<'alloc> String<'alloc> { /// [`new_in`]: String::new_in #[inline(always)] pub fn with_capacity_in(capacity: usize, allocator: &'alloc Allocator) -> String<'alloc> { - Self(BumpaloString::with_capacity_in(capacity, allocator)) + Self(BumpaloString::with_capacity_in(capacity, allocator.bump())) } /// Construct a new [`String`] from a string slice. @@ -74,7 +74,7 @@ impl<'alloc> String<'alloc> { /// ``` #[inline(always)] pub fn from_str_in(s: &str, allocator: &'alloc Allocator) -> String<'alloc> { - Self(BumpaloString::from_str_in(s, allocator)) + Self(BumpaloString::from_str_in(s, allocator.bump())) } /// Convert `Vec` into [`String`]. @@ -157,7 +157,7 @@ impl<'alloc> String<'alloc> { allocator: &'alloc Allocator, ) -> String<'alloc> { // SAFETY: Safety conditions of this method are the same as `BumpaloString`'s method - Self(BumpaloString::from_raw_parts_in(buf, length, capacity, allocator)) + Self(BumpaloString::from_raw_parts_in(buf, length, capacity, allocator.bump())) } /// Convert this `String<'alloc>` into an `&'alloc str`. This is analogous to diff --git a/crates/oxc_allocator/src/vec.rs b/crates/oxc_allocator/src/vec.rs index 214e5bb1122bc..3589a4b32c46d 100644 --- a/crates/oxc_allocator/src/vec.rs +++ b/crates/oxc_allocator/src/vec.rs @@ -56,7 +56,7 @@ impl<'alloc, T> Vec<'alloc, T> { /// ``` #[inline(always)] pub fn new_in(allocator: &'alloc Allocator) -> Self { - Self(ManuallyDrop::new(InnerVec::new_in(allocator))) + Self(ManuallyDrop::new(InnerVec::new_in(allocator.bump()))) } /// Constructs a new, empty `Vec` with at least the specified capacity @@ -108,7 +108,7 @@ impl<'alloc, T> Vec<'alloc, T> { /// ``` #[inline(always)] pub fn with_capacity_in(capacity: usize, allocator: &'alloc Allocator) -> Self { - Self(ManuallyDrop::new(InnerVec::with_capacity_in(capacity, allocator))) + Self(ManuallyDrop::new(InnerVec::with_capacity_in(capacity, allocator.bump()))) } /// Create a new [`Vec`] whose elements are taken from an iterator and @@ -120,7 +120,7 @@ impl<'alloc, T> Vec<'alloc, T> { let iter = iter.into_iter(); let hint = iter.size_hint(); let capacity = hint.1.unwrap_or(hint.0); - let mut vec = ManuallyDrop::new(InnerVec::with_capacity_in(capacity, &**allocator)); + let mut vec = ManuallyDrop::new(InnerVec::with_capacity_in(capacity, allocator.bump())); vec.extend(iter); Self(vec) } @@ -149,7 +149,7 @@ impl<'alloc, T> Vec<'alloc, T> { // `ptr` was allocated with correct size for `[T; N]`. // `len` and `capacity` are both `N`. // Allocated size cannot be larger than `isize::MAX`, or `Box::new_in` would have failed. - let vec = unsafe { InnerVec::from_raw_parts_in(ptr, N, N, &**allocator) }; + let vec = unsafe { InnerVec::from_raw_parts_in(ptr, N, N, allocator.bump()) }; Self(ManuallyDrop::new(vec)) }