Skip to content

refactor(semantic): add ArenaIdentHashMap and replace IdentHashMap callsites #18425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Boshen wants to merge 1 commit into from
Closed

refactor(semantic): add ArenaIdentHashMap and replace IdentHashMap callsites #18425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 69 additions & 50 deletions crates/oxc_allocator/src/hash_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#![expect(clippy::inline_always)]

use std::{
hash::Hash,
hash::{BuildHasher, Hash},
mem::ManuallyDrop,
ops::{Deref, DerefMut},
};
Expand All @@ -28,14 +28,14 @@ pub use hashbrown::{

use crate::Allocator;

type FxHashMap<'alloc, K, V> = hashbrown::HashMap<K, V, FxBuildHasher, &'alloc Bump>;
type InnerHashMap<'alloc, K, V, H> = hashbrown::HashMap<K, V, H, &'alloc Bump>;

/// A hash map without `Drop`, that uses [`FxHasher`] to hash keys, and stores data in arena allocator.
/// A hash map without `Drop`, that stores data in arena allocator.
///
/// Just a thin wrapper around [`hashbrown::HashMap`], which disables the `Drop` implementation.
///
/// All APIs are the same, except create a [`HashMap`] with
/// either [`new_in`](HashMap::new_in) or [`with_capacity_in`](HashMap::with_capacity_in).
/// All APIs are the same, except create a [`HashMapImpl`] with
/// either [`new_in`](HashMapImpl::new_in) or [`with_capacity_in`](HashMapImpl::with_capacity_in).
///
/// # No `Drop`s
///
Expand All @@ -45,79 +45,95 @@ type FxHashMap<'alloc, K, V> = hashbrown::HashMap<K, V, FxBuildHasher, &'alloc B
/// Therefore, it would produce a memory leak if you allocated [`Drop`] types into the arena
/// which own memory allocations outside the arena.
///
/// Static checks make this impossible to do. [`HashMap::new_in`] and all other methods which create
/// a [`HashMap`] will refuse to compile if either key or value is a [`Drop`] type.
/// Static checks make this impossible to do. [`HashMapImpl::new_in`] and all other methods which create
/// a [`HashMapImpl`] will refuse to compile if either key or value is a [`Drop`] type.
pub struct HashMapImpl<'alloc, K, V, H: BuildHasher>(
pub(crate) ManuallyDrop<InnerHashMap<'alloc, K, V, H>>,
);

impl<K, V, H: BuildHasher> std::fmt::Debug for HashMapImpl<'_, K, V, H>
where
K: std::fmt::Debug,
V: std::fmt::Debug,
{
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_map().entries(self.0.iter()).finish()
}
}

/// A hash map without `Drop`, that uses [`FxHasher`] to hash keys, and stores data in arena allocator.
///
/// This is a type alias for [`HashMapImpl`] with [`FxBuildHasher`].
///
/// [`FxHasher`]: rustc_hash::FxHasher
#[derive(Debug)]
pub struct HashMap<'alloc, K, V>(pub(crate) ManuallyDrop<FxHashMap<'alloc, K, V>>);
pub type HashMap<'alloc, K, V> = HashMapImpl<'alloc, K, V, FxBuildHasher>;

/// SAFETY: Even though `Bump` is not `Sync`, we can make `HashMap<K, V>` `Sync` if both `K` and `V`
/// SAFETY: Even though `Bump` is not `Sync`, we can make `HashMapImpl<K, V, H>` `Sync` if both `K` and `V`
/// are `Sync` because:
///
/// 1. No public methods allow access to the `&Bump` that `HashMap` contains (in `hashbrown::HashMap`),
/// so user cannot illegally obtain 2 `&Bump`s on different threads via `HashMap`.
/// 1. No public methods allow access to the `&Bump` that `HashMapImpl` contains (in `hashbrown::HashMap`),
/// so user cannot illegally obtain 2 `&Bump`s on different threads via `HashMapImpl`.
///
/// 2. All internal methods which access the `&Bump` take a `&mut self`.
/// `&mut HashMap` cannot be transferred across threads, and nor can an owned `HashMap`
/// (`HashMap` is not `Send`).
/// Therefore these methods taking `&mut self` can be sure they're not operating on a `HashMap`
/// `&mut HashMapImpl` cannot be transferred across threads, and nor can an owned `HashMapImpl`
/// (`HashMapImpl` is not `Send`).
/// Therefore these methods taking `&mut self` can be sure they're not operating on a `HashMapImpl`
/// which has been moved across threads.
///
/// Note: `HashMap` CANNOT be `Send`, even if `K` and `V` are `Send`, because that would allow 2 `HashMap`s
/// Note: `HashMapImpl` CANNOT be `Send`, even if `K` and `V` are `Send`, because that would allow 2 `HashMapImpl`s
/// on different threads to both allocate into same arena simultaneously. `Bump` is not thread-safe,
/// and this would be undefined behavior.
///
/// ### Soundness holes
///
/// This is not actually fully sound. There are 2 holes I (@overlookmotel) am aware of:
///
/// 1. `allocator` method, which does allow access to the `&Bump` that `HashMap` contains.
/// 1. `allocator` method, which does allow access to the `&Bump` that `HashMapImpl` contains.
/// 2. `Clone` impl on `hashbrown::HashMap`, which may perform allocations in the arena, given only a
/// `&self` reference.
///
/// [`HashMap::allocator`] prevents accidental access to the underlying method of `hashbrown::HashMap`,
/// and `clone` called on a `&HashMap` clones the `&HashMap` reference, not the `HashMap` itself (harmless).
/// [`HashMapImpl::allocator`] prevents accidental access to the underlying method of `hashbrown::HashMap`,
/// and `clone` called on a `&HashMapImpl` clones the `&HashMapImpl` reference, not the `HashMapImpl` itself (harmless).
/// But both can be accessed via explicit `Deref` (`hash_map.deref().allocator()` or `hash_map.deref().clone()`),
/// so we don't have complete soundness.
///
/// To close these holes we need to remove `Deref` and `DerefMut` impls on `HashMap`, and instead add
/// methods to `HashMap` itself which pass on calls to the inner `hashbrown::HashMap`.
/// To close these holes we need to remove `Deref` and `DerefMut` impls on `HashMapImpl`, and instead add
/// methods to `HashMapImpl` itself which pass on calls to the inner `hashbrown::HashMap`.
///
/// TODO: Fix these holes.
/// TODO: Remove any other methods that currently allow performing allocations with only a `&self` reference.
unsafe impl<K: Sync, V: Sync> Sync for HashMap<'_, K, V> {}
unsafe impl<K: Sync, V: Sync, H: BuildHasher> Sync for HashMapImpl<'_, K, V, H> {}

// TODO: `IntoIter`, `Drain`, and other consuming iterators provided by `hashbrown` are `Drop`.
// Wrap them in `ManuallyDrop` to prevent that.

impl<'alloc, K, V> HashMap<'alloc, K, V> {
impl<'alloc, K, V, H: BuildHasher + Default> HashMapImpl<'alloc, K, V, H> {
/// Const assertions that `K` and `V` are not `Drop`.
/// Must be referenced in all methods which create a `HashMap`.
/// Must be referenced in all methods which create a `HashMapImpl`.
const ASSERT_K_AND_V_ARE_NOT_DROP: () = {
assert!(
!std::mem::needs_drop::<K>(),
"Cannot create a HashMap<K, V> where K is a Drop type"
"Cannot create a HashMapImpl<K, V, H> where K is a Drop type"
);
assert!(
!std::mem::needs_drop::<V>(),
"Cannot create a HashMap<K, V> where V is a Drop type"
"Cannot create a HashMapImpl<K, V, H> where V is a Drop type"
);
};

/// Creates an empty [`HashMap`]. It will be allocated with the given allocator.
/// Creates an empty [`HashMapImpl`]. It will be allocated with the given allocator.
///
/// The hash map is initially created with a capacity of 0, so it will not allocate
/// until it is first inserted into.
#[inline(always)]
pub fn new_in(allocator: &'alloc Allocator) -> Self {
const { Self::ASSERT_K_AND_V_ARE_NOT_DROP };

let inner = FxHashMap::with_hasher_in(FxBuildHasher, allocator.bump());
let inner = InnerHashMap::with_hasher_in(H::default(), allocator.bump());
Self(ManuallyDrop::new(inner))
}

/// Creates an empty [`HashMap`] with the specified capacity. It will be allocated with the given allocator.
/// Creates an empty [`HashMapImpl`] with the specified capacity. It will be allocated with the given allocator.
///
/// The hash map will be able to hold at least capacity elements without reallocating.
/// If capacity is 0, the hash map will not allocate.
Expand All @@ -126,11 +142,11 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> {
const { Self::ASSERT_K_AND_V_ARE_NOT_DROP };

let inner =
FxHashMap::with_capacity_and_hasher_in(capacity, FxBuildHasher, allocator.bump());
InnerHashMap::with_capacity_and_hasher_in(capacity, H::default(), allocator.bump());
Self(ManuallyDrop::new(inner))
}

/// Create a new [`HashMap`] whose elements are taken from an iterator and
/// Create a new [`HashMapImpl`] whose elements are taken from an iterator and
/// allocated in the given `allocator`.
///
/// This is behaviorally identical to [`FromIterator::from_iter`].
Expand All @@ -150,13 +166,14 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> {
// This follows `hashbrown::HashMap`'s `from_iter` implementation.
//
// This is a trade-off:
// * Negative: If lower bound is too low, the `HashMap` may have to grow and reallocate during `for_each` loop.
// * Negative: If lower bound is too low, the `HashMapImpl` may have to grow and reallocate during `for_each` loop.
// * Positive: Avoids potential large over-allocation for iterators where upper bound may be a large over-estimate
// e.g. filter iterators.
let capacity = iter.size_hint().0;
let map = FxHashMap::with_capacity_and_hasher_in(capacity, FxBuildHasher, allocator.bump());
let map =
InnerHashMap::with_capacity_and_hasher_in(capacity, H::default(), allocator.bump());
// Wrap in `ManuallyDrop` *before* calling `for_each`, so compiler doesn't insert unnecessary code
// to drop the `FxHashMap` in case of a panic in iterator's `next` method
// to drop the `InnerHashMap` in case of a panic in iterator's `next` method
let mut map = ManuallyDrop::new(map);

iter.for_each(|(k, v)| {
Expand All @@ -165,7 +182,9 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> {

Self(map)
}
}

impl<'alloc, K, V, H: BuildHasher> HashMapImpl<'alloc, K, V, H> {
/// Creates a consuming iterator visiting all the keys in arbitrary order.
///
/// The map cannot be used after calling this. The iterator element type is `K`.
Expand All @@ -186,7 +205,7 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> {

/// Calling this method produces a compile-time panic.
///
/// This method would be unsound, because [`HashMap`] is `Sync`, and the underlying allocator
/// This method would be unsound, because [`HashMapImpl`] is `Sync`, and the underlying allocator
/// (`Bump`) is not `Sync`.
///
/// This method exists only to block access as much as possible to the underlying
Expand All @@ -203,23 +222,23 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> {
}

// Provide access to all `hashbrown::HashMap`'s methods via deref
impl<'alloc, K, V> Deref for HashMap<'alloc, K, V> {
type Target = FxHashMap<'alloc, K, V>;
impl<'alloc, K, V, H: BuildHasher> Deref for HashMapImpl<'alloc, K, V, H> {
type Target = InnerHashMap<'alloc, K, V, H>;

#[inline]
fn deref(&self) -> &Self::Target {
&self.0
}
}

impl<'alloc, K, V> DerefMut for HashMap<'alloc, K, V> {
impl<'alloc, K, V, H: BuildHasher> DerefMut for HashMapImpl<'alloc, K, V, H> {
#[inline]
fn deref_mut(&mut self) -> &mut FxHashMap<'alloc, K, V> {
fn deref_mut(&mut self) -> &mut InnerHashMap<'alloc, K, V, H> {
&mut self.0
}
}

impl<'alloc, K, V> IntoIterator for HashMap<'alloc, K, V> {
impl<'alloc, K, V, H: BuildHasher> IntoIterator for HashMapImpl<'alloc, K, V, H> {
type IntoIter = IntoIter<K, V, &'alloc Bump>;
type Item = (K, V);

Expand All @@ -236,38 +255,38 @@ impl<'alloc, K, V> IntoIterator for HashMap<'alloc, K, V> {
}
}

impl<'alloc, 'i, K, V> IntoIterator for &'i HashMap<'alloc, K, V> {
type IntoIter = <&'i FxHashMap<'alloc, K, V> as IntoIterator>::IntoIter;
impl<'alloc, 'i, K, V, H: BuildHasher> IntoIterator for &'i HashMapImpl<'alloc, K, V, H> {
type IntoIter = <&'i InnerHashMap<'alloc, K, V, H> as IntoIterator>::IntoIter;
type Item = (&'i K, &'i V);

/// Creates an iterator over the entries of a `HashMap` in arbitrary order.
/// Creates an iterator over the entries of a `HashMapImpl` in arbitrary order.
///
/// The iterator element type is `(&'a K, &'a V)`.
///
/// Return the same [`Iter`] struct as by the `iter` method on [`HashMap`].
/// Return the same [`Iter`] struct as by the `iter` method on [`HashMapImpl`].
#[inline(always)]
fn into_iter(self) -> Self::IntoIter {
self.0.iter()
}
}

impl<'alloc, 'i, K, V> IntoIterator for &'i mut HashMap<'alloc, K, V> {
type IntoIter = <&'i mut FxHashMap<'alloc, K, V> as IntoIterator>::IntoIter;
impl<'alloc, 'i, K, V, H: BuildHasher> IntoIterator for &'i mut HashMapImpl<'alloc, K, V, H> {
type IntoIter = <&'i mut InnerHashMap<'alloc, K, V, H> as IntoIterator>::IntoIter;
type Item = (&'i K, &'i mut V);

/// Creates an iterator over the entries of a `HashMap` in arbitrary order
/// Creates an iterator over the entries of a `HashMapImpl` in arbitrary order
/// with mutable references to the values.
///
/// The iterator element type is `(&'a K, &'a mut V)`.
///
/// Return the same [`IterMut`] struct as by the `iter_mut` method on [`HashMap`].
/// Return the same [`IterMut`] struct as by the `iter_mut` method on [`HashMapImpl`].
#[inline(always)]
fn into_iter(self) -> Self::IntoIter {
self.0.iter_mut()
}
}

impl<K, V> PartialEq for HashMap<'_, K, V>
impl<K, V, H: BuildHasher> PartialEq for HashMapImpl<'_, K, V, H>
where
K: Eq + Hash,
V: PartialEq,
Expand All @@ -278,7 +297,7 @@ where
}
}

impl<K, V> Eq for HashMap<'_, K, V>
impl<K, V, H: BuildHasher> Eq for HashMapImpl<'_, K, V, H>
where
K: Eq + Hash,
V: Eq,
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_allocator/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ pub use bitset::BitSet;
pub use boxed::Box;
pub use clone_in::CloneIn;
pub use convert::{FromIn, IntoIn};
pub use hash_map::HashMap;
pub use hash_map::{HashMap, HashMapImpl};
pub use hash_set::HashSet;
#[cfg(feature = "pool")]
pub use pool::*;
Expand Down
8 changes: 7 additions & 1 deletion crates/oxc_ast/src/ast_builder_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use std::borrow::Cow;

use oxc_allocator::{Allocator, AllocatorAccessor, Box, FromIn, IntoIn, Vec};
use oxc_span::{Atom, SPAN, Span};
use oxc_span::{Atom, Ident, SPAN, Span};
use oxc_syntax::{
comment_node::CommentNodeId, number::NumberBase, operator::UnaryOperator, scope::ScopeId,
};
Expand Down Expand Up @@ -104,6 +104,12 @@ impl<'a> AstBuilder<'a> {
Atom::from_in(value, self.allocator)
}

/// Allocate an [`Ident`] from a string slice.
#[inline]
pub fn ident(self, value: &str) -> Ident<'a> {
Ident::from_in(value, self.allocator)
}

/// Allocate an [`Atom`] from an array of string slices.
#[inline]
pub fn atom_from_strs_array<const N: usize>(self, strings: [&str; N]) -> Atom<'a> {
Expand Down
18 changes: 9 additions & 9 deletions crates/oxc_isolated_declarations/src/declaration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl<'a> IsolatedDeclarations<'a> {
) -> Option<VariableDeclarator<'a>> {
if decl.id.is_destructuring_pattern() {
decl.id.bound_names(&mut |id| {
if !check_binding || self.scope.has_value_reference(&id.name) {
if !check_binding || self.scope.has_value_reference(id.name) {
self.error(binding_element_export(id.span));
}
});
Expand All @@ -57,7 +57,7 @@ impl<'a> IsolatedDeclarations<'a> {

if check_binding
&& let Some(name) = decl.id.get_identifier_name()
&& !self.scope.has_value_reference(&name)
&& !self.scope.has_value_reference(name)
{
return None;
}
Expand Down Expand Up @@ -167,7 +167,7 @@ impl<'a> IsolatedDeclarations<'a> {
match decl {
Declaration::FunctionDeclaration(func) => {
let needs_transform = !check_binding
|| func.id.as_ref().is_some_and(|id| self.scope.has_value_reference(&id.name));
|| func.id.as_ref().is_some_and(|id| self.scope.has_value_reference(id.name));
needs_transform
.then(|| Declaration::FunctionDeclaration(self.transform_function(func, None)))
}
Expand All @@ -176,12 +176,12 @@ impl<'a> IsolatedDeclarations<'a> {
.map(Declaration::VariableDeclaration),
Declaration::ClassDeclaration(decl) => {
let needs_transform = !check_binding
|| decl.id.as_ref().is_some_and(|id| self.scope.has_reference(&id.name));
|| decl.id.as_ref().is_some_and(|id| self.scope.has_reference(id.name));
needs_transform
.then(|| Declaration::ClassDeclaration(self.transform_class(decl, None)))
}
Declaration::TSTypeAliasDeclaration(alias_decl) => {
if !check_binding || self.scope.has_reference(&alias_decl.id.name) {
if !check_binding || self.scope.has_reference(alias_decl.id.name) {
let mut decl = decl.clone_in(self.ast.allocator);
self.visit_declaration(&mut decl);
Some(decl)
Expand All @@ -190,7 +190,7 @@ impl<'a> IsolatedDeclarations<'a> {
}
}
Declaration::TSInterfaceDeclaration(interface_decl) => {
if !check_binding || self.scope.has_reference(&interface_decl.id.name) {
if !check_binding || self.scope.has_reference(interface_decl.id.name) {
let mut decl = decl.clone_in(self.ast.allocator);
self.visit_declaration(&mut decl);
Some(decl)
Expand All @@ -199,7 +199,7 @@ impl<'a> IsolatedDeclarations<'a> {
}
}
Declaration::TSEnumDeclaration(enum_decl) => {
if !check_binding || self.scope.has_reference(&enum_decl.id.name) {
if !check_binding || self.scope.has_reference(enum_decl.id.name) {
Some(self.transform_ts_enum_declaration(enum_decl))
} else {
None
Expand All @@ -210,7 +210,7 @@ impl<'a> IsolatedDeclarations<'a> {
|| matches!(
&decl.id,
TSModuleDeclarationName::Identifier(ident)
if self.scope.has_reference(&ident.name)
if self.scope.has_reference(ident.name)
)
{
Some(Declaration::TSModuleDeclaration(
Expand All @@ -224,7 +224,7 @@ impl<'a> IsolatedDeclarations<'a> {
Some(Declaration::TSGlobalDeclaration(decl.clone_in(self.ast.allocator)))
}
Declaration::TSImportEqualsDeclaration(decl) => {
if !check_binding || self.scope.has_reference(&decl.id.name) {
if !check_binding || self.scope.has_reference(decl.id.name) {
Some(Declaration::TSImportEqualsDeclaration(decl.clone_in(self.ast.allocator)))
} else {
None
Expand Down
Loading
Loading