Skip to content

Commit f539f64

Browse files
committed
perf(allocator): remove Arc from AllocatorPool (#11760)
Follow-on after #11736. Remove the overhead of `Arc` from `AllocatorPool`. This does introduce more lifetimes in linter, which is unwelcome, but as well as removing the synchronization overhead of `Arc`, I think it reflects the semantics of what we're trying to do more correctly. `AllocatorPool` solely owns the `Vec<Allocator>` pool. It doesn't need to (and shouldn't) share ownership of it with `AllocatorGuard`s, which `Arc` does. So `AllocatorGuard`s can just hold a reference to the `AllocatorPool`.
1 parent 17f5dbe commit f539f64

File tree

2 files changed

+40
-28
lines changed

2 files changed

+40
-28
lines changed

crates/oxc_allocator/src/pool.rs

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use std::{
22
mem::ManuallyDrop,
33
ops::{Deref, DerefMut},
4-
sync::{Arc, Mutex},
4+
sync::Mutex,
55
};
66

77
use crate::Allocator;
@@ -11,14 +11,14 @@ use crate::Allocator;
1111
/// Internally uses a `Vec` protected by a `Mutex` to store available allocators.
1212
#[derive(Default)]
1313
pub struct AllocatorPool {
14-
allocators: Arc<Mutex<Vec<Allocator>>>,
14+
allocators: Mutex<Vec<Allocator>>,
1515
}
1616

1717
impl AllocatorPool {
1818
/// Creates a new `AllocatorPool` pre-filled with the given number of default `Allocator` instances.
1919
pub fn new(size: usize) -> AllocatorPool {
2020
let allocators = std::iter::repeat_with(Allocator::default).take(size).collect();
21-
AllocatorPool { allocators: Arc::new(Mutex::new(allocators)) }
21+
AllocatorPool { allocators: Mutex::new(allocators) }
2222
}
2323

2424
/// Retrieves an `Allocator` from the pool, or creates a new one if the pool is empty.
@@ -34,41 +34,50 @@ impl AllocatorPool {
3434
allocators.pop().unwrap_or_default()
3535
};
3636

37-
AllocatorGuard {
38-
allocator: ManuallyDrop::new(allocator),
39-
pool: Arc::clone(&self.allocators),
40-
}
37+
AllocatorGuard { allocator: ManuallyDrop::new(allocator), pool: self }
38+
}
39+
40+
/// Add an `Allocator` to the pool.
41+
///
42+
/// The `Allocator` should be empty, ready to be re-used.
43+
///
44+
/// # Panics
45+
///
46+
/// Panics if the underlying mutex is poisoned.
47+
fn add(&self, allocator: Allocator) {
48+
let mut allocators = self.allocators.lock().unwrap();
49+
allocators.push(allocator);
4150
}
4251
}
4352

4453
/// A guard object representing exclusive access to an `Allocator` from the pool.
4554
///
4655
/// On drop, the `Allocator` is reset and returned to the pool.
47-
pub struct AllocatorGuard {
56+
pub struct AllocatorGuard<'alloc_pool> {
4857
allocator: ManuallyDrop<Allocator>,
49-
pool: Arc<Mutex<Vec<Allocator>>>,
58+
pool: &'alloc_pool AllocatorPool,
5059
}
5160

52-
impl Deref for AllocatorGuard {
61+
impl Deref for AllocatorGuard<'_> {
5362
type Target = Allocator;
5463

5564
fn deref(&self) -> &Self::Target {
5665
&self.allocator
5766
}
5867
}
5968

60-
impl DerefMut for AllocatorGuard {
69+
impl DerefMut for AllocatorGuard<'_> {
6170
fn deref_mut(&mut self) -> &mut Self::Target {
6271
&mut self.allocator
6372
}
6473
}
6574

66-
impl Drop for AllocatorGuard {
75+
impl Drop for AllocatorGuard<'_> {
76+
/// Return `Allocator` back to the pool.
6777
fn drop(&mut self) {
68-
// SAFETY: we're taking ownership and promise not to drop `allocator` again.
78+
// SAFETY: After taking ownership of the `Allocator`, we do not touch the `ManuallyDrop` again
6979
let mut allocator = unsafe { ManuallyDrop::take(&mut self.allocator) };
7080
allocator.reset();
71-
let mut allocators = self.pool.lock().unwrap();
72-
allocators.push(allocator);
81+
self.pool.add(allocator);
7382
}
7483
}

crates/oxc_linter/src/service/runtime.rs

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -47,16 +47,16 @@ pub struct Runtime<'l> {
4747
}
4848

4949
/// Output of `Runtime::process_path`
50-
struct ModuleProcessOutput {
50+
struct ModuleProcessOutput<'alloc_pool> {
5151
/// All paths in `Runtime` are stored as `OsStr`, because `OsStr` hash is faster
5252
/// than `Path` - go checkout their source code.
5353
path: Arc<OsStr>,
54-
processed_module: ProcessedModule,
54+
processed_module: ProcessedModule<'alloc_pool>,
5555
}
5656

5757
/// A module processed from a path
5858
#[derive(Default)]
59-
struct ProcessedModule {
59+
struct ProcessedModule<'alloc_pool> {
6060
/// Module records of source sections, or diagnostics if parsing failed on that section.
6161
///
6262
/// Modules with special extensions such as .vue could contain multiple source sections (see `PartialLoader::PartialLoader`).
@@ -71,7 +71,7 @@ struct ProcessedModule {
7171
///
7272
/// Note that `content` is `Some` even if parsing is unsuccessful as long as the source to lint is valid utf-8.
7373
/// It is designed this way to cover the case where some but not all the sections fail to parse.
74-
content: Option<ModuleContent>,
74+
content: Option<ModuleContent<'alloc_pool>>,
7575
}
7676

7777
struct ResolvedModuleRequest {
@@ -86,18 +86,18 @@ struct ResolvedModuleRecord {
8686
}
8787

8888
self_cell! {
89-
struct ModuleContent {
90-
owner: ModuleContentOwner,
89+
struct ModuleContent<'alloc_pool> {
90+
owner: ModuleContentOwner<'alloc_pool>,
9191
#[not_covariant]
9292
dependent: SectionContents,
9393
}
9494
}
9595
// Safety: dependent borrows from owner. They're safe to be sent together.
96-
unsafe impl Send for ModuleContent {}
96+
unsafe impl Send for ModuleContent<'_> {}
9797

98-
struct ModuleContentOwner {
98+
struct ModuleContentOwner<'alloc_pool> {
9999
source_text: String,
100-
allocator: AllocatorGuard,
100+
allocator: AllocatorGuard<'alloc_pool>,
101101
}
102102

103103
/// source text and semantic for each source section. They are in the same order as `ProcessedModule.section_module_records`
@@ -113,13 +113,16 @@ struct SectionContent<'a> {
113113
///
114114
/// A `ModuleWithContent` is generated for each path in `runtime.paths`. It's basically the same
115115
/// as `ProcessedModule`, except `content` is non-Option.
116-
struct ModuleToLint {
116+
struct ModuleToLint<'alloc_pool> {
117117
path: Arc<OsStr>,
118118
section_module_records: SmallVec<[Result<Arc<ModuleRecord>, Vec<OxcDiagnostic>>; 1]>,
119-
content: ModuleContent,
119+
content: ModuleContent<'alloc_pool>,
120120
}
121-
impl ModuleToLint {
122-
fn from_processed_module(path: Arc<OsStr>, processed_module: ProcessedModule) -> Option<Self> {
121+
impl<'alloc_pool> ModuleToLint<'alloc_pool> {
122+
fn from_processed_module(
123+
path: Arc<OsStr>,
124+
processed_module: ProcessedModule<'alloc_pool>,
125+
) -> Option<Self> {
123126
processed_module.content.map(|content| Self {
124127
path,
125128
section_module_records: processed_module

0 commit comments

Comments
 (0)