feat(oxc_linter): reuse allocators

[camc314]

No description provided.

[camc314]

• feat(oxc_linter): reuse allocators #11736 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

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-hq]

CodSpeed Instrumentation Performance Report

Merging #11736 will not alter performance

_{Comparing c/06-16-feat_oxc_linter_reuse_allocators (38dc614) with main (584844c)}

Summary

✅ 38 untouched benchmarks

[Boshen]

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 😁

[overlookmotel]

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.
• factory can be removed.
• PoolDrop trait 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>>>?

[overlookmotel]

You mentioned earlier that using a pool produces no noticeable perf improvement. Was that on CodSpeed benchmarks? Or a local wallclock benchmark?

[camc314]

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)

[camc314]

hyperfine --warmup 5 '../../Boshen/oxc/target/release/oxlint -A all --silent' '../../Boshen/oxc/target/release/oxlint-reuse-allocators -A all --silent' -i
Benchmark 1: ../../Boshen/oxc/target/release/oxlint -A all --silent
  Time (mean ± σ):     129.5 ms ±   2.4 ms    [User: 894.6 ms, System: 198.4 ms]
  Range (min … max):   126.4 ms … 138.3 ms    22 runs

  Warning: Ignoring non-zero exit code.

Benchmark 2: ../../Boshen/oxc/target/release/oxlint-reuse-allocators -A all --silent
  Time (mean ± σ):     127.3 ms ±   1.7 ms    [User: 884.9 ms, System: 188.0 ms]
  Range (min … max):   125.1 ms … 132.3 ms    23 runs

  Warning: Ignoring non-zero exit code.

Summary
  ../../Boshen/oxc/target/release/oxlint-reuse-allocators -A all --silent ran
    1.02 ± 0.02 times faster than ../../Boshen/oxc/target/release/oxlint -A all --silent

[camc314]

@overlookmotel thanks for the detailed review, mind taking another look? I think i've applied all of your suggestions

[overlookmotel]

Reviewing now. Did you find any difference in perf between Vec and VecDeque?

[camc314]

Reviewing now. Did you find any difference in perf between Vec and VecDeque?

thanks. Not really the perf seemed within benchmarking noise

[overlookmotel]

Great. All my comments are just to do with naming.

[Sysix]

I don't have the expertise here. LGTM.

[overlookmotel]

Merge activity

• Jun 16, 8:17 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Jun 16, 8:17 PM UTC: overlookmotel added this pull request to the Graphite merge queue.
• Jun 16, 8:24 PM UTC: Merged by the Graphite merge queue.