Skip to content

Commit

Permalink
perf(linter): make all rules share a diagnostics vec (#5806)
Browse files Browse the repository at this point in the history
## 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`

```rust
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.
  • Loading branch information
DonIsaac committed Sep 17, 2024
1 parent e04841c commit 3725d5d
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 22 deletions.
40 changes: 31 additions & 9 deletions crates/oxc_linter/src/context/host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::{cell::RefCell, path::Path, rc::Rc, sync::Arc};
use crate::{
config::LintConfig,
disable_directives::{DisableDirectives, DisableDirectivesBuilder},
fixer::FixKind,
fixer::{FixKind, Message},
frameworks,
options::{LintOptions, LintPlugins},
utils, FrameworkFlags, RuleWithSeverity,
Expand Down Expand Up @@ -33,9 +33,13 @@ use super::{plugin_name_to_prefix, LintContext};
/// - [Flyweight Pattern](https://en.wikipedia.org/wiki/Flyweight_pattern)
#[must_use]
#[non_exhaustive]
pub struct ContextHost<'a> {
pub(crate) struct ContextHost<'a> {
pub(super) semantic: Rc<Semantic<'a>>,
pub(super) disable_directives: DisableDirectives<'a>,
/// Diagnostics reported by the linter.
///
/// Contains diagnostics for all rules across a single file.
diagnostics: RefCell<Vec<Message<'a>>>,
/// Whether or not to apply code fixes during linting. Defaults to
/// [`FixKind::None`] (no fixing).
///
Expand All @@ -56,6 +60,8 @@ impl<'a> ContextHost<'a> {
semantic: Rc<Semantic<'a>>,
options: LintOptions,
) -> Self {
const DIAGNOSTICS_INITIAL_CAPACITY: usize = 512;

// We should always check for `semantic.cfg()` being `Some` since we depend on it and it is
// unwrapped without any runtime checks after construction.
assert!(
Expand All @@ -72,6 +78,7 @@ impl<'a> ContextHost<'a> {
Self {
semantic,
disable_directives,
diagnostics: RefCell::new(Vec::with_capacity(DIAGNOSTICS_INITIAL_CAPACITY)),
fix: options.fix,
file_path,
config: Arc::new(LintConfig::default()),
Expand All @@ -82,7 +89,7 @@ impl<'a> ContextHost<'a> {
}

#[inline]
pub(crate) fn with_config(mut self, config: &Arc<LintConfig>) -> Self {
pub fn with_config(mut self, config: &Arc<LintConfig>) -> Self {
self.config = Arc::clone(config);
self
}
Expand All @@ -108,14 +115,26 @@ impl<'a> ContextHost<'a> {
self.semantic.source_type()
}

pub(crate) fn spawn(self: Rc<Self>, rule: &RuleWithSeverity) -> LintContext<'a> {
const DIAGNOSTICS_INITIAL_CAPACITY: usize = 128;
#[inline]
pub(super) fn push_diagnostic(&self, diagnostic: Message<'a>) {
self.diagnostics.borrow_mut().push(diagnostic);
}

/// Take all diagnostics collected during linting.
pub fn take_diagnostics(&self) -> Vec<Message<'a>> {
// NOTE: diagnostics are only ever borrowed here and in push_diagnostic.
// The latter drops the reference as soon as the function returns, so
// this should never panic.
let mut messages = self.diagnostics.borrow_mut();
std::mem::take(&mut *messages)
}

pub fn spawn(self: Rc<Self>, rule: &RuleWithSeverity) -> LintContext<'a> {
let rule_name = rule.name();
let plugin_name = self.map_jest(rule.plugin_name(), rule_name);

LintContext {
parent: self,
diagnostics: RefCell::new(Vec::with_capacity(DIAGNOSTICS_INITIAL_CAPACITY)),
current_rule_name: rule_name,
current_plugin_name: plugin_name,
current_plugin_prefix: plugin_name_to_prefix(plugin_name),
Expand All @@ -127,11 +146,8 @@ impl<'a> ContextHost<'a> {

#[cfg(test)]
pub(crate) fn spawn_for_test(self: Rc<Self>) -> LintContext<'a> {
const DIAGNOSTICS_INITIAL_CAPACITY: usize = 128;

LintContext {
parent: Rc::clone(&self),
diagnostics: RefCell::new(Vec::with_capacity(DIAGNOSTICS_INITIAL_CAPACITY)),
current_rule_name: "",
current_plugin_name: "eslint",
current_plugin_prefix: "eslint",
Expand Down Expand Up @@ -175,3 +191,9 @@ impl<'a> ContextHost<'a> {
self
}
}

impl<'a> From<ContextHost<'a>> for Vec<Message<'a>> {
fn from(ctx_host: ContextHost<'a>) -> Self {
ctx_host.diagnostics.into_inner()
}
}
15 changes: 3 additions & 12 deletions crates/oxc_linter/src/context/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#![allow(rustdoc::private_intra_doc_links)] // useful for intellisense
mod host;

use std::{cell::RefCell, path::Path, rc::Rc};
use std::{path::Path, rc::Rc};

use oxc_cfg::ControlFlowGraph;
use oxc_diagnostics::{OxcDiagnostic, Severity};
Expand All @@ -18,19 +18,14 @@ use crate::{
AllowWarnDeny, FrameworkFlags, OxlintEnv, OxlintGlobals, OxlintSettings,
};

pub use host::ContextHost;
pub(crate) use host::ContextHost;

#[derive(Clone)]
#[must_use]
pub struct LintContext<'a> {
/// Shared context independent of the rule being linted.
parent: Rc<ContextHost<'a>>,

/// Diagnostics reported by the linter.
///
/// Contains diagnostics for all rules across all files.
diagnostics: RefCell<Vec<Message<'a>>>,

// states
current_plugin_name: &'static str,
current_plugin_prefix: &'static str,
Expand Down Expand Up @@ -158,10 +153,6 @@ impl<'a> LintContext<'a> {

/* Diagnostics */

pub fn into_message(self) -> Vec<Message<'a>> {
self.diagnostics.into_inner()
}

fn add_diagnostic(&self, mut message: Message<'a>) {
if self.parent.disable_directives.contains(self.current_rule_name, message.span()) {
return;
Expand All @@ -179,7 +170,7 @@ impl<'a> LintContext<'a> {
message.error = message.error.with_severity(self.severity);
}

self.diagnostics.borrow_mut().push(message);
self.parent.push_diagnostic(message);
}

/// Report a lint rule violation.
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_linter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ impl Linter {
}
}

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

/// # Panics
Expand Down

0 comments on commit 3725d5d

Please sign in to comment.