Skip to content

Commit 79470e3

Browse files
authored
[isort] Avoid constructing glob::Patterns for literal known modules (#25123)
Summary -- For repositories with many known isort modules and a large number of extended configuration files, Ruff can consume a lot of memory. This is especially problematic in an LSP context, where this memory hangs around for the duration of the session. Codex and I tracked this down to the overhead of constructing `glob::Pattern`s for each `KnownModule`. I initially tried a more complicated approach of interning each `Pattern` during configuration resolution, but I suspected that the common case for projects with large numbers of known modules would be plain string literal patterns that don't need the `glob` infrastructure. This assumption was supported in the repository I was testing on, which saw a ~7x decrease in peak memory usage, which was only very slightly less than I saw with the interned version. There's probably something more general we could do here, as this comment alludes to: https://github.com/astral-sh/ruff/blob/c09080468ab6ef2d3e674043d310f21ec074d219/crates/ruff_workspace/src/resolver.rs#L311-L314 but this seemed like an easy first step for a known issue. Test plan -- A couple of new unit tests for the `IdentifierPattern` enum (as well as existing `isort` tests) and some manual testing on a large repo
1 parent 2522549 commit 79470e3

4 files changed

Lines changed: 85 additions & 13 deletions

File tree

crates/ruff_linter/src/rules/isort/categorize.rs

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use serde::{Deserialize, Serialize};
99
use strum_macros::EnumIter;
1010

1111
use crate::package::PackageRoot;
12+
use crate::settings::types::IdentifierPattern;
1213
use crate::warn_user_once;
1314
use ruff_macros::CacheKey;
1415
use ruff_python_ast::PythonVersion;
@@ -283,20 +284,20 @@ pub(crate) fn categorize_imports<'a>(
283284
#[derive(Debug, Clone, Default, CacheKey)]
284285
pub struct KnownModules {
285286
/// A map of known modules to their section.
286-
known: Vec<(glob::Pattern, ImportSection)>,
287+
known: Vec<(IdentifierPattern, ImportSection)>,
287288
/// Whether any of the known modules are submodules (e.g., `foo.bar`, as opposed to `foo`).
288289
has_submodules: bool,
289290
}
290291

291292
impl KnownModules {
292293
pub fn new(
293-
first_party: Vec<glob::Pattern>,
294-
third_party: Vec<glob::Pattern>,
295-
local_folder: Vec<glob::Pattern>,
296-
standard_library: Vec<glob::Pattern>,
297-
user_defined: FxHashMap<String, Vec<glob::Pattern>>,
294+
first_party: Vec<IdentifierPattern>,
295+
third_party: Vec<IdentifierPattern>,
296+
local_folder: Vec<IdentifierPattern>,
297+
standard_library: Vec<IdentifierPattern>,
298+
user_defined: FxHashMap<String, Vec<IdentifierPattern>>,
298299
) -> Self {
299-
let known: Vec<(glob::Pattern, ImportSection)> = user_defined
300+
let known: Vec<(IdentifierPattern, ImportSection)> = user_defined
300301
.into_iter()
301302
.flat_map(|(section, modules)| {
302303
modules
@@ -395,8 +396,8 @@ impl KnownModules {
395396
}
396397

397398
/// Return the list of user-defined modules, indexed by section.
398-
pub fn user_defined(&self) -> FxHashMap<&str, Vec<&glob::Pattern>> {
399-
let mut user_defined: FxHashMap<&str, Vec<&glob::Pattern>> = FxHashMap::default();
399+
pub fn user_defined(&self) -> FxHashMap<&str, Vec<&IdentifierPattern>> {
400+
let mut user_defined: FxHashMap<&str, Vec<&IdentifierPattern>> = FxHashMap::default();
400401
for (module, section) in &self.known {
401402
if let ImportSection::UserDefined(section_name) = section {
402403
user_defined

crates/ruff_linter/src/rules/isort/mod.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,7 @@ mod tests {
317317
use crate::registry::Rule;
318318
use crate::rules::isort::categorize::{ImportSection, KnownModules};
319319
use crate::settings::LinterSettings;
320+
use crate::settings::types::IdentifierPattern;
320321
use crate::test::{test_path, test_resource_path};
321322

322323
use super::categorize::ImportType;
@@ -389,8 +390,8 @@ mod tests {
389390
Ok(())
390391
}
391392

392-
fn pattern(pattern: &str) -> glob::Pattern {
393-
glob::Pattern::new(pattern).unwrap()
393+
fn pattern(pattern: &str) -> IdentifierPattern {
394+
IdentifierPattern::new(pattern).unwrap()
394395
}
395396

396397
#[test_case(Path::new("separate_subpackage_first_and_third_party_imports.py"))]

crates/ruff_linter/src/settings/types.rs

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -693,7 +693,53 @@ impl Display for RequiredVersion {
693693
/// For reference pep8-naming uses
694694
/// [`fnmatch`](https://docs.python.org/3/library/fnmatch.html) for
695695
/// pattern matching.
696-
pub type IdentifierPattern = glob::Pattern;
696+
///
697+
/// Literal patterns without glob metacharacters fall back on string
698+
/// comparison to avoid the overhead of constructing a [`glob::Pattern`].
699+
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, CacheKey)]
700+
pub enum IdentifierPattern {
701+
Literal(String),
702+
Glob(Box<glob::Pattern>),
703+
}
704+
705+
impl IdentifierPattern {
706+
pub fn new(pattern: &str) -> Result<Self, glob::PatternError> {
707+
// `]` is only special inside `[...]`, which necessarily includes `[`.
708+
if pattern.contains(['?', '*', '[']) {
709+
Ok(Self::Glob(Box::new(glob::Pattern::new(pattern)?)))
710+
} else {
711+
Ok(Self::Literal(pattern.to_string()))
712+
}
713+
}
714+
715+
pub fn matches(&self, candidate: &str) -> bool {
716+
match self {
717+
Self::Literal(literal) => literal == candidate,
718+
Self::Glob(pattern) => pattern.matches(candidate),
719+
}
720+
}
721+
722+
pub fn as_str(&self) -> &str {
723+
match self {
724+
Self::Literal(literal) => literal,
725+
Self::Glob(pattern) => pattern.as_str(),
726+
}
727+
}
728+
}
729+
730+
impl Display for IdentifierPattern {
731+
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
732+
f.write_str(self.as_str())
733+
}
734+
}
735+
736+
impl FromStr for IdentifierPattern {
737+
type Err = glob::PatternError;
738+
739+
fn from_str(pattern: &str) -> Result<Self, Self::Err> {
740+
Self::new(pattern)
741+
}
742+
}
697743

698744
/// Like [`PerFile`] but with string globs compiled to [`GlobMatcher`]s for more efficient usage.
699745
#[derive(Debug, Clone)]
@@ -943,8 +989,32 @@ impl Display for CompiledPerFileTargetVersionList {
943989

944990
#[cfg(test)]
945991
mod tests {
992+
use super::IdentifierPattern;
993+
946994
#[test]
947995
fn default_python_version_works() {
948996
super::PythonVersion::default();
949997
}
998+
999+
#[test]
1000+
fn identifier_pattern_matches_literals_exactly() {
1001+
let pattern = IdentifierPattern::new("package.module").unwrap();
1002+
1003+
assert!(pattern.matches("package.module"));
1004+
assert!(!pattern.matches("package"));
1005+
assert!(!pattern.matches("package.module.extra"));
1006+
}
1007+
1008+
#[test]
1009+
fn identifier_pattern_preserves_glob_matching() {
1010+
let pattern = IdentifierPattern::new("package.*").unwrap();
1011+
1012+
assert!(pattern.matches("package.module"));
1013+
assert!(!pattern.matches("other.module"));
1014+
}
1015+
1016+
#[test]
1017+
fn identifier_pattern_rejects_invalid_globs() {
1018+
assert!(IdentifierPattern::new("package[").is_err());
1019+
}
9501020
}

crates/ruff_workspace/src/options.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2963,7 +2963,7 @@ impl IsortOptions {
29632963
let sections = self.sections.unwrap_or_default();
29642964

29652965
// Verify that `sections` doesn't contain any built-in sections.
2966-
let sections: FxHashMap<String, Vec<glob::Pattern>> = sections
2966+
let sections: FxHashMap<String, Vec<IdentifierPattern>> = sections
29672967
.into_iter()
29682968
.filter_map(|(section, modules)| match section {
29692969
ImportSection::Known(section) => {

0 commit comments

Comments
 (0)