perf(linter): make all rules share a diagnostics vec

[DonIsaac]

What This PR Does

Each time a lint rule is run on a file, it will now store diagnostics in a
shared vec instead of having its own list. This is done by moving diagnostics
from LintContext to ContextHost.

The motivating line of code can be found in Linter::run in
oxc_linter/src/lib.rs#145

rules.into_iter().flat_map(|(_, ctx)| ctx.into_message()).collect::<Vec<_>>()

Each LintContext allocates a new vec, and pushes diagnostics into it. After
all run-related methods have been run, a new vec is created and those
diagnostics are copied into it. This is O(n+1) allocations and O(n) copies,
not to mention that allocations for the original vecs cannot be re-used since
those vecs aren't dropped until after the new one is created.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[DonIsaac]

• fix(linter): remove all* remaining "Disallow <foo>" messages #5812
• feat(linter): make typescript/no-duplicate-enum-values a correctness rule #5810
• feat(span): impl From<Atom<'a>> for Atom #5809
• fix(linter): improve diagnostic messages for various lint rules #5808
• fix(linter): plugin prefix name for eslint-plugin-node #5807
• perf(linter): make all rules share a diagnostics vec #5806 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @DonIsaac and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #5806 will not alter performance

_{Comparing don/09-16-perf_linter_make_all_rules_share_a_diagnostics_vec (3725d5d) with main (e04841c)}

Summary

✅ 29 untouched benchmarks

[overlookmotel]

Just to check my understanding of the linter is correct:

• For each individual file, all rules run on the same thread.
• Each file may run on a different thread.

Do I have that right? If so, this change does make some sense.

But... benchmarks seem to show this actually making things slightly slower rather than faster (-0.2% to -0.5% change). It's hard to tell if that's just noise in benchmarks. You may want to add a dummy commit to re-run the benchmarks, and then revert the commit to run them again. If the perf drop shows consistently across 3 runs, then it's probably real.

0.5% is not much, so it's no big deal either way. But it seems counter-productive to make a change specifically to improve perf, if it actually does the opposite.

If the perf drop is real, explanation may be that the additional cost of RefCell is outweighing the gain of reducing memory copies.

Presumably in real world usage, most rules will report 0 errors on 99% of files, because the programmer will fix (or disable) any lint errors pretty quickly. And if there are no errors, then there's no memory to copy, so this change doesn't gain anything, but on negative side it incurs the cost of RefCells.

(We could have our cake and eat it by replacing RefCell with zero-cost GhostCell, but that's a bit of an undertaking due to the weird lifetimes.)

PS: Unrelated, but I also wonder if initial capacity of 128 is too high. Do files in general produce that many errors? If I'm right that 0 is the most common number of errors, it might be better not to allocate any initial capacity at all.

[DonIsaac]

Just to check my understanding of the linter is correct:

• For each individual file, all rules run on the same thread.
• Each file may run on a different thread.

Do I have that right? If so, this change does make some sense.

Yes

But... benchmarks seem to show this actually making things slightly slower rather than faster (-0.2% to -0.5% change). It's hard to tell if that's just noise in benchmarks. You may want to add a dummy commit to re-run the benchmarks, and then revert the commit to run them again. If the perf drop shows consistently across 3 runs, then it's probably real.

0.5% is not much, so it's no big deal either way. But it seems counter-productive to make a change specifically to improve perf, if it actually does the opposite.

I'll investigate those regressions. Locally, I was seeing a 7% perf improvement on the RadixUIAdoptionSection benchmark. Maybe it has to do with the number of rules?

PS: Unrelated, but I also wonder if initial capacity of 128 is too high. Do files in general produce that many errors? If I'm right that 0 is the most common number of errors, it might be better not to allocate any initial capacity at all.

I'll play around with the numbers and see what works best

[DonIsaac]

I got a ~0.8% improvement by increasing the initial capacity to 512. This makes sense, as before each rule got a list with an initial capacity of 128.

[DonIsaac]

...And now it's even worse in CI... that's odd, I'll keep looking into it.

[graphite-app]

Merge activity

• Sep 17, 1:45 AM EDT: Boshen added this pull request to the Graphite merge queue.
• Sep 17, 1:50 AM EDT: Boshen merged this pull request with the Graphite merge queue.

[overlookmotel]

@DonIsaac I'm not sure if you intended this to be merged, given that you were still investigating.

Locally, I was seeing a 7% perf improvement on the RadixUIAdoptionSection benchmark.

This is really odd, and worth investigating. If our CodSpeed benchmarks are that far off reality, we should try to fix that (or at least understand in what circumstances it happens).

When you ran it locally, did you run with the same rules enabled as on CI? (i.e. was it exact like-for-like comparison?)

How many errors does running all the rules on RadixUIAdoptionSection produce?

If my assumption is correct that in real world most commonly the linter will produce 0 errors, and our benchmark fixtures produce loads, then perhaps we need new benchmark fixtures!