From 3725d5d44e6f162331f19e2879332135041514c7 Mon Sep 17 00:00:00 2001 From: DonIsaac <22823424+DonIsaac@users.noreply.github.com> Date: Tue, 17 Sep 2024 05:46:15 +0000 Subject: [PATCH] perf(linter): make all rules share a diagnostics vec (#5806) ## 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::>() ``` 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. --- crates/oxc_linter/src/context/host.rs | 40 +++++++++++++++++++++------ crates/oxc_linter/src/context/mod.rs | 15 ++-------- crates/oxc_linter/src/lib.rs | 2 +- 3 files changed, 35 insertions(+), 22 deletions(-) diff --git a/crates/oxc_linter/src/context/host.rs b/crates/oxc_linter/src/context/host.rs index b84d91146735e..2ead2913c21f7 100644 --- a/crates/oxc_linter/src/context/host.rs +++ b/crates/oxc_linter/src/context/host.rs @@ -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, @@ -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>, pub(super) disable_directives: DisableDirectives<'a>, + /// Diagnostics reported by the linter. + /// + /// Contains diagnostics for all rules across a single file. + diagnostics: RefCell>>, /// Whether or not to apply code fixes during linting. Defaults to /// [`FixKind::None`] (no fixing). /// @@ -56,6 +60,8 @@ impl<'a> ContextHost<'a> { semantic: Rc>, 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!( @@ -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()), @@ -82,7 +89,7 @@ impl<'a> ContextHost<'a> { } #[inline] - pub(crate) fn with_config(mut self, config: &Arc) -> Self { + pub fn with_config(mut self, config: &Arc) -> Self { self.config = Arc::clone(config); self } @@ -108,14 +115,26 @@ impl<'a> ContextHost<'a> { self.semantic.source_type() } - pub(crate) fn spawn(self: Rc, 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> { + // 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, 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), @@ -127,11 +146,8 @@ impl<'a> ContextHost<'a> { #[cfg(test)] pub(crate) fn spawn_for_test(self: Rc) -> 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", @@ -175,3 +191,9 @@ impl<'a> ContextHost<'a> { self } } + +impl<'a> From> for Vec> { + fn from(ctx_host: ContextHost<'a>) -> Self { + ctx_host.diagnostics.into_inner() + } +} diff --git a/crates/oxc_linter/src/context/mod.rs b/crates/oxc_linter/src/context/mod.rs index c4c8ead2d43cc..53589f2a47a75 100644 --- a/crates/oxc_linter/src/context/mod.rs +++ b/crates/oxc_linter/src/context/mod.rs @@ -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}; @@ -18,7 +18,7 @@ use crate::{ AllowWarnDeny, FrameworkFlags, OxlintEnv, OxlintGlobals, OxlintSettings, }; -pub use host::ContextHost; +pub(crate) use host::ContextHost; #[derive(Clone)] #[must_use] @@ -26,11 +26,6 @@ pub struct LintContext<'a> { /// Shared context independent of the rule being linted. parent: Rc>, - /// Diagnostics reported by the linter. - /// - /// Contains diagnostics for all rules across all files. - diagnostics: RefCell>>, - // states current_plugin_name: &'static str, current_plugin_prefix: &'static str, @@ -158,10 +153,6 @@ impl<'a> LintContext<'a> { /* Diagnostics */ - pub fn into_message(self) -> Vec> { - 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; @@ -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. diff --git a/crates/oxc_linter/src/lib.rs b/crates/oxc_linter/src/lib.rs index 02f51d39d335f..c685dbb202915 100644 --- a/crates/oxc_linter/src/lib.rs +++ b/crates/oxc_linter/src/lib.rs @@ -142,7 +142,7 @@ impl Linter { } } - rules.into_iter().flat_map(|(_, ctx)| ctx.into_message()).collect::>() + ctx_host.take_diagnostics() } /// # Panics