feat(oxc_linter): reuse allocators #11736
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Instrumentation Performance ReportMerging #11736 will not alter performanceComparing Summary
|
f419c1c to
81d43ed
Compare
There was a problem hiding this comment.
I wonder how much max RSS this reduces. https://github.com/oxc-project/bench-javascript-parser-written-in-rust/blob/456e5ab52ccb0196ffa104a1822f7009c753b94b/memory.sh#L9
Merge if it does 😁
There was a problem hiding this comment.
A few thoughts:
Generics
Does Pool need to be generic? We're only using it for Allocators. The generics make it harder to understand what's going on.
Pool<T>->AllocatorPool.PoolGuard<T: PoolDrop>->AllocatorGuard.factorycan be removed.PoolDroptrait can be removed.
Remove Option
AllocatorGuard could contain an Allocator instead of Option<Allocator>. That'd remove unwrap from deref and deref_mut.
Then in impl Drop for AllocatorGuard, use std::mem::take(&mut self.allocator) to get the owned Allocator.
Or, slightly better, wrap the Allocator in ManuallyDrop, so compiler will not run drop on the empty allocator you just replaced the real one with. Compiler will probably then be able to elide the std::mem::take call, because it'll see the default empty allocator it creates is never accessed.
pub struct AllocatorGuard {
allocator: ManuallyDrop<Allocator>,
pool: Arc<Mutex<VecDeque<T>>>,
}
impl Drop for AllocatorGuard {
fn drop(&mut self) {
let mut allocator = ManuallyDrop::into_inner(
mem::take(&mut self.allocator)
);
allocator.reset();
let mut items = self.pool.lock().unwrap();
items.push_back(item);
}
}VecDeque
Is purpose of using VecDeque instead of Vec to try to avoid cache invalidation?
I don't know if that'll work because:
a. Allocators are fairly small.
b. When pool is empty (common case once all the threads have started linting files if plugin-import is not enabled), end of the queue is same as the start.
c. When pool is full, start and end are next to each other.
So start and end of the queue may end up being in same cache line anyway.
When plugin-import is active, and you're generating loads of allocators, it's generally preferable to re-use the Allocator which was most recently returned to the pool, because it's the most likely one to still be warm in cache. A Vec which you push and pop from at same end would give that advantage.
I'm not sure about how these various factors interact, so it may be that a VecDeque is indeed better. But I think Vec is at least trying, and measuring if any difference on e.g. VSCode repo with plugin-import enabled.
Reference to AllocatorPool in AllocatorGuard
I don't know if this makes any difference, but could AllocatorGuard contain an Arc<AllocatorPool> instead of Arc<Mutex<VecDeque<Allocator>>>?
|
You mentioned earlier that using a pool produces no noticeable perf improvement. Was that on CodSpeed benchmarks? Or a local wallclock benchmark? |
didn't bother checking codspeed as it runs on single files. I was using hyperfine on vscode repo (no import plugin) |
81d43ed to
c03c257
Compare
3abf5a9 to
e31ce54
Compare
|
|
@overlookmotel thanks for the detailed review, mind taking another look? I think i've applied all of your suggestions |
|
Reviewing now. Did you find any difference in perf between |
thanks. Not really the perf seemed within benchmarking noise |
e31ce54 to
2b55a4f
Compare
Merge activity
|
2b55a4f to
38dc614
Compare
Follow-on after #11736. Pure refactor. Move all code related to `AllocatorGuard` to be together, so it's easier to read. Also rearrange `std` imports.
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.2.0] - 2025-06-19 ### 🚀 Features - 8c341a2 sema/check: Ts setters cannot have initializers (#11695) (Don Isaac) - 38dc614 oxc_linter: Reuse allocators (#11736) (camc314) - bf8263d playground: Allow specifying a JSON string as the linter config (#11710) (Nicholas Rayburn) - 0b4261b vscode: Add `oxc.requireConfig` configuration (#11700) (Sysix) - 52ecc87 linter: Implement import/extensions (#11548) (Tyler Earls) - 094b81c language_server: Add `unusedDisableDirectives` option (#11645) (Sysix) ### 🐛 Bug Fixes - 3d88eeb linter/no-console: False negative when `console.*` methods are used as args to functions (#11790) (camc314) - c80e405 linter/no-new-wrappers: Fix panic in fixer with multi byte chars (#11773) (camc314) - e58a0b0 linter: Panic in unicorn/consistent-function-scoping (#11772) (camc314) - 80c87d4 linter: Typo in typescript/consistent-index-object-style (#11744) (camc314) - ff775e9 linter/consistent-function-scoping: Descriptive diagnostic labels (#11682) (Don Isaac) - 989634a linter/no-inner-declaration: False negative with for loops (#11692) (camc314) - b272b91 linter/no-undef: False negative with unresolved ref after type ref (#11721) (camc314) - 6252275 linter: Panic in import/extensions with empty file names (#11720) (camc314) - f34e432 linter: Use fixer::noop in dangerous cases for eslint/no-var (#11693) (camc314) - 6c2b41c linter/consistent-function-scoping: Allow functions in TS modules/namespaces (#11681) (Don Isaac) - 2ca1c70 linter/exhaustive-deps: False positive with TS Non null assertion operator (#11690) (camc314) - ee15f7d linter: False negative in typescript/prefer-function-type (#11674) (camc314) - abd0441 linter: Add missing menuitemradio and menutitemcheckbox roles (#11651) (Daniel Flynn) - 8776301 linter/no-inner-declarations: Flag `var` statement as body of `for` loop (#11632) (overlookmotel) ### 🚜 Refactor - 5ca3d04 ast: Add `TSArrayType` as `AstKind` (#11745) (camchenry) - abdbaa9 language_server: Use rule name directly from OxcCode instead of parsing out of the stringified version of OxcCode (#11714) (Nicholas Rayburn) - 219adcc ast: Don't generate AstKind for ArrayExpressionElement (#11684) (Ulrich Stark) - c1be6b8 linter: Shorten Span construction (#11686) (Ulrich Stark) - 4ca659c linter: Cleanup typescript/prefer-function-type (#11672) (Brad Dunbar) - 8e30c5f ast: Don't generate AstKind for ForStatementInit (#11617) (Ulrich Stark) ### 📚 Documentation - ea6ce9d linter: Fix typo in import/no-namespace (#11741) (camc314) - 8b6076e linter: Document options for the `typescript/array-type` rule (#11665) (yefan) ### ⚡ Performance - f539f64 allocator: Remove `Arc` from `AllocatorPool` (#11760) (overlookmotel) - cfdc518 linter/no-inner-declarations: Move work to cold path (#11746) (overlookmotel) - 7c0fff7 linter: Skip running `consistent-function-scoping` on `.d.ts` files (#11739) (camc314) - b34c6f6 parser,semantic: Improve handling of diagnostics (#11641) (Boshen) - 2cd786b linter/no-inner-declarations: Remove unnecessary code and reduce branches (#11633) (overlookmotel) ### 🧪 Testing - 44a9df8 linter: Update testsuite for `no-undef` (#11706) (Sysix) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
No description provided.