Improve performance of `Owners` rule #16563

Eric-Arellano · 2022-08-17T23:00:21Z

Possibly will close #16122.

@stuhood found in #16122 (comment) that we were spending substantial time in Rust parsing the same globs in the stateless function matches_filespec.

This improves the situation by introducing FilespecsMatcher, which allows us to persist the globs parsing over time. Crucially, FilespecsMatcher eagerly parses the globs in its constructor, unlike PathGlobs currently being lazy. Then, we use a @memoized_method on each SourcesField instance to avoid ever creating the matcher more than necessary.

Before:

❯ hyperfine -w 2 -r 10 './pants --no-pantsd --no-buildsense-enable --no-anonymous-telemetry-enabled dependencies ::'
  Time (mean ± σ):      6.562 s ±  0.153 s    [User: 7.354 s, System: 1.516 s]
  Range (min … max):    6.420 s …  6.878 s    10 runs

After:

❯ hyperfine -w 2 -r 10 './pants --no-pantsd --no-buildsense-enable --no-anonymous-telemetry-enabled dependencies ::'
  Time (mean ± σ):      6.460 s ±  0.065 s    [User: 7.301 s, System: 1.447 s]
  Range (min … max):    6.340 s …  6.576 s    10 runs

(Note, we expect the performance to be better in repos with the shape described at #16122 (comment))

Relationship to `PathGlobs`

We also considered changing PathGlobs to eagerly parse, and then defining PathGlobs.matches(), rather than introducing the new type FilespecsMatcher.

I believe FilespecsMatcher is a clearer API: it is only used for in-memory checks if globs match paths, whereas PathGlobs is used for on-disk.

Notably, the Rust code only overlaps in how the exclude patterns are ignored. So, making this split allows us to simplify PathGlobs.

A downside of having separate types is it's harder to share the duplicate parsing of the exclude patterns. This seems fine, given that the include patterns are separate code paths.

(See #16564 for improving PathGlobs to also eagerly parse.)

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

[ci skip-build-wheels] # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

Eric-Arellano · 2022-08-17T23:01:49Z

src/rust/engine/fs/src/glob_matching.rs

-  fn parse_patterns_from_include(
-    include: &[PathGlobIncludeEntry],
-  ) -> Result<Vec<glob::Pattern>, String> {


Inlined below

Eric-Arellano · 2022-08-17T23:02:41Z

src/rust/engine/fs/src/lib.rs

-  /// to check if it may optimize the snapshot subset operation on this tree,
-  /// or need to check for excluded files/directories.
-  ///
-  pub fn maybe_is_parent_of_ignored_path(&self, path: &Path) -> bool {


Eric-Arellano · 2022-08-17T23:02:56Z

src/rust/engine/src/externs/fs.rs

-impl PyPathGlobs {
-  fn parse(self) -> PyResult<PreparedPathGlobs> {


Eric-Arellano · 2022-08-17T23:03:43Z

src/rust/engine/src/externs/fs.rs

+      .0
+      .include_globs()
+      .iter()
+      .map(|pattern| format!("{pattern}"))


@stuhood is there a better way to call Display on the value?

pattern.to_string() calls Display.

Eric-Arellano · 2022-08-17T23:04:02Z

src/rust/engine/src/externs/fs.rs

+  }
+
+  fn matches(&self, paths: Vec<String>, py: Python) -> PyResult<Vec<String>> {
+    py.allow_threads(|| {


@stuhood has suggested it may be more efficient to remove this

Would need to do some microbenchmarking, yea.

The two cases are:

taking paths: Vec<&str>, which would avoid copying the inputs, but would require us to hold the GIL, or

keeping as is

You could maybe temporarily add a loop around the match calls in src/python/pants/source/filespec_test.py assert_rule_matches, and then compare that way. It's not a completely realistic use case, but it would be straightforward. I expect that it's not worth doing anything more involved though.

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood

Thanks!

docs/markdown/Writing Plugins/common-plugin-tasks/plugin-upgrade-guide.md

stuhood · 2022-08-18T19:59:37Z

src/rust/engine/fs/src/glob_matching.rs

-  /// owners detection of deleted files (see #6790 and #5636 for more info). The lazy filesystem
-  /// traversal in expand is (currently) too expensive to use for that in-memory matching (such as
-  /// via MemFS).


Preserving the explanation of why we have two implementations of matching (because the lazy impl is too slow for this usecase) would be good. It may not be true forever.

stuhood · 2022-08-18T20:01:15Z

src/rust/engine/src/externs/fs.rs

+      .0
+      .include_globs()
+      .iter()
+      .map(|pattern| format!("{pattern}"))


pattern.to_string() calls Display.

stuhood · 2022-08-18T20:04:12Z

src/rust/engine/src/externs/fs.rs

+  }
+
+  fn matches(&self, paths: Vec<String>, py: Python) -> PyResult<Vec<String>> {
+    py.allow_threads(|| {


Would need to do some microbenchmarking, yea.

The two cases are:

taking paths: Vec<&str>, which would avoid copying the inputs, but would require us to hold the GIL, or

keeping as is

You could maybe temporarily add a loop around the match calls in src/python/pants/source/filespec_test.py assert_rule_matches, and then compare that way. It's not a completely realistic use case, but it would be straightforward. I expect that it's not worth doing anything more involved though.

stuhood · 2022-08-18T20:05:50Z

src/rust/engine/fs/src/glob_matching.rs

+    let includes = includes
+      .iter()
+      .map(|glob| {
+        PathGlob::normalize_pattern(glob).and_then(|components| {
+          let normalized_pattern: PathBuf = components.into_iter().collect();
+          Pattern::new(normalized_pattern.to_str().unwrap())
+            .map_err(|e| format!("Could not parse {:?} as a glob: {:?}", glob, e))
+        })
+      })
+      .collect::<Result<Vec<_>, String>>()?;
+    let excludes = GitignoreStyleExcludes::create(excludes)?;


Both of these should be executed with the GIL released I think.

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

#16563. [ci skip-rust]

@stuhood

Possibly will close pantsbuild#16122. @stuhood found in pantsbuild#16122 (comment) that we were spending substantial time in Rust parsing the same globs in the stateless function `matches_filespec`. This improves the situation by introducing `FilespecsMatcher`, which allows us to persist the globs parsing over time. Crucially, `FilespecsMatcher` eagerly parses the globs in its constructor, unlike `PathGlobs` currently being lazy. Then, we use a `@memoized_method` on each `SourcesField` instance to avoid ever creating the matcher more than necessary. Before: ``` ❯ hyperfine -w 2 -r 10 './pants --no-pantsd --no-buildsense-enable --no-anonymous-telemetry-enabled dependencies ::' Time (mean ± σ): 6.562 s ± 0.153 s [User: 7.354 s, System: 1.516 s] Range (min … max): 6.420 s … 6.878 s 10 runs ``` After: ``` ❯ hyperfine -w 2 -r 10 './pants --no-pantsd --no-buildsense-enable --no-anonymous-telemetry-enabled dependencies ::' Time (mean ± σ): 6.460 s ± 0.065 s [User: 7.301 s, System: 1.447 s] Range (min … max): 6.340 s … 6.576 s 10 runs ``` (Note, we expect the performance to be better in repos with the shape described at pantsbuild#16122 (comment)) ## Relationship to `PathGlobs` We also considered changing `PathGlobs` to eagerly parse, and then defining `PathGlobs.matches()`, rather than introducing the new type `FilespecsMatcher`. I believe `FilespecsMatcher` is a clearer API: it is only used for in-memory checks if globs match paths, whereas `PathGlobs` is used for on-disk. Notably, the Rust code only overlaps in how the exclude patterns are ignored. So, making this split allows us to simplify `PathGlobs`. A downside of having separate types is it's harder to share the duplicate parsing of the exclude patterns. This seems fine, given that the include patterns are separate code paths. (See pantsbuild#16564 for improving `PathGlobs` to also eagerly parse.) # Conflicts: # docs/markdown/Writing Plugins/common-plugin-tasks/plugin-upgrade-guide.md

@stuhood

Possibly will close pantsbuild#16122. @stuhood found in pantsbuild#16122 (comment) that we were spending substantial time in Rust parsing the same globs in the stateless function `matches_filespec`. This improves the situation by introducing `FilespecsMatcher`, which allows us to persist the globs parsing over time. Crucially, `FilespecsMatcher` eagerly parses the globs in its constructor, unlike `PathGlobs` currently being lazy. Then, we use a `@memoized_method` on each `SourcesField` instance to avoid ever creating the matcher more than necessary. Before: ``` ❯ hyperfine -w 2 -r 10 './pants --no-pantsd --no-buildsense-enable --no-anonymous-telemetry-enabled dependencies ::' Time (mean ± σ): 6.562 s ± 0.153 s [User: 7.354 s, System: 1.516 s] Range (min … max): 6.420 s … 6.878 s 10 runs ``` After: ``` ❯ hyperfine -w 2 -r 10 './pants --no-pantsd --no-buildsense-enable --no-anonymous-telemetry-enabled dependencies ::' Time (mean ± σ): 6.460 s ± 0.065 s [User: 7.301 s, System: 1.447 s] Range (min … max): 6.340 s … 6.576 s 10 runs ``` (Note, we expect the performance to be better in repos with the shape described at pantsbuild#16122 (comment)) We also considered changing `PathGlobs` to eagerly parse, and then defining `PathGlobs.matches()`, rather than introducing the new type `FilespecsMatcher`. I believe `FilespecsMatcher` is a clearer API: it is only used for in-memory checks if globs match paths, whereas `PathGlobs` is used for on-disk. Notably, the Rust code only overlaps in how the exclude patterns are ignored. So, making this split allows us to simplify `PathGlobs`. A downside of having separate types is it's harder to share the duplicate parsing of the exclude patterns. This seems fine, given that the include patterns are separate code paths. (See pantsbuild#16564 for improving `PathGlobs` to also eagerly parse.) [ci skip-build-wheels] [ci skip-rust] [ci skip-build-wheels]

@stuhood

Possibly will close #16122. @stuhood found in #16122 (comment) that we were spending substantial time in Rust parsing the same globs in the stateless function `matches_filespec`. This improves the situation by introducing `FilespecsMatcher`, which allows us to persist the globs parsing over time. Crucially, `FilespecsMatcher` eagerly parses the globs in its constructor, unlike `PathGlobs` currently being lazy. Then, we use a `@memoized_method` on each `SourcesField` instance to avoid ever creating the matcher more than necessary. Before: ``` ❯ hyperfine -w 2 -r 10 './pants --no-pantsd --no-buildsense-enable --no-anonymous-telemetry-enabled dependencies ::' Time (mean ± σ): 6.562 s ± 0.153 s [User: 7.354 s, System: 1.516 s] Range (min … max): 6.420 s … 6.878 s 10 runs ``` After: ``` ❯ hyperfine -w 2 -r 10 './pants --no-pantsd --no-buildsense-enable --no-anonymous-telemetry-enabled dependencies ::' Time (mean ± σ): 6.460 s ± 0.065 s [User: 7.301 s, System: 1.447 s] Range (min … max): 6.340 s … 6.576 s 10 runs ``` (Note, we expect the performance to be better in repos with the shape described at #16122 (comment)) ## Relationship to `PathGlobs` We also considered changing `PathGlobs` to eagerly parse, and then defining `PathGlobs.matches()`, rather than introducing the new type `FilespecsMatcher`. I believe `FilespecsMatcher` is a clearer API: it is only used for in-memory checks if globs match paths, whereas `PathGlobs` is used for on-disk. Notably, the Rust code only overlaps in how the exclude patterns are ignored. So, making this split allows us to simplify `PathGlobs`. A downside of having separate types is it's harder to share the duplicate parsing of the exclude patterns. This seems fine, given that the include patterns are separate code paths. (See #16564 for improving `PathGlobs` to also eagerly parse.)

@stuhood

Possibly will close pantsbuild#16122. @stuhood found in pantsbuild#16122 (comment) that we were spending substantial time in Rust parsing the same globs in the stateless function `matches_filespec`. This improves the situation by introducing `FilespecsMatcher`, which allows us to persist the globs parsing over time. Crucially, `FilespecsMatcher` eagerly parses the globs in its constructor, unlike `PathGlobs` currently being lazy. Then, we use a `@memoized_method` on each `SourcesField` instance to avoid ever creating the matcher more than necessary. Before: ``` ❯ hyperfine -w 2 -r 10 './pants --no-pantsd --no-buildsense-enable --no-anonymous-telemetry-enabled dependencies ::' Time (mean ± σ): 6.562 s ± 0.153 s [User: 7.354 s, System: 1.516 s] Range (min … max): 6.420 s … 6.878 s 10 runs ``` After: ``` ❯ hyperfine -w 2 -r 10 './pants --no-pantsd --no-buildsense-enable --no-anonymous-telemetry-enabled dependencies ::' Time (mean ± σ): 6.460 s ± 0.065 s [User: 7.301 s, System: 1.447 s] Range (min … max): 6.340 s … 6.576 s 10 runs ``` (Note, we expect the performance to be better in repos with the shape described at pantsbuild#16122 (comment)) ## Relationship to `PathGlobs` We also considered changing `PathGlobs` to eagerly parse, and then defining `PathGlobs.matches()`, rather than introducing the new type `FilespecsMatcher`. I believe `FilespecsMatcher` is a clearer API: it is only used for in-memory checks if globs match paths, whereas `PathGlobs` is used for on-disk. Notably, the Rust code only overlaps in how the exclude patterns are ignored. So, making this split allows us to simplify `PathGlobs`. A downside of having separate types is it's harder to share the duplicate parsing of the exclude patterns. This seems fine, given that the include patterns are separate code paths. (See pantsbuild#16564 for improving `PathGlobs` to also eagerly parse.)

pantsbuild#16563. [ci skip-rust]

Don't use PathGlobs for match_path_globs pyfunction

e854d34

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano added needs-cherrypick category:performance labels Aug 17, 2022

Eric-Arellano requested a review from stuhood August 17, 2022 23:00

Introduce Python type

aa03cf3

[ci skip-build-wheels] # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

Eric-Arellano force-pushed the improve-perf-of-owners-filespec branch from 0356b15 to aa03cf3 Compare August 17, 2022 23:01

Eric-Arellano commented Aug 17, 2022

View reviewed changes

Eric-Arellano mentioned this pull request Aug 17, 2022

Make PathGlobs parsing of globs eager #16564

Open

Add to changelog

0dc797a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood approved these changes Aug 18, 2022

View reviewed changes

stuhood reviewed Aug 18, 2022

View reviewed changes

Review feedback

39ffdd0

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano merged commit d55493f into pantsbuild:main Aug 18, 2022

Eric-Arellano deleted the improve-perf-of-owners-filespec branch August 18, 2022 22:47

Eric-Arellano mentioned this pull request Aug 19, 2022

Fix deprecation to use FilespecMatcher in fmt.py #16583

Merged

Eric-Arellano added a commit that referenced this pull request Aug 19, 2022

Fix deprecation to use FilespecMatcher in fmt.py (#16583)

0e233d3

#16563. [ci skip-rust]

Eric-Arellano removed the needs-cherrypick label Aug 24, 2022

cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022

Fix deprecation to use FilespecMatcher in fmt.py (pantsbuild#16583)

618625b

pantsbuild#16563. [ci skip-rust]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of `Owners` rule #16563

Improve performance of `Owners` rule #16563

Eric-Arellano commented Aug 17, 2022 •

edited

Loading

Eric-Arellano Aug 17, 2022

Eric-Arellano Aug 17, 2022

Eric-Arellano Aug 17, 2022

Eric-Arellano Aug 17, 2022

stuhood Aug 18, 2022

Eric-Arellano Aug 17, 2022

stuhood Aug 18, 2022

stuhood left a comment

stuhood Aug 18, 2022

stuhood Aug 18, 2022

stuhood Aug 18, 2022

stuhood Aug 18, 2022

		impl PyPathGlobs {
		fn parse(self) -> PyResult<PreparedPathGlobs> {

Improve performance of Owners rule #16563

Improve performance of Owners rule #16563

Conversation

Eric-Arellano commented Aug 17, 2022 • edited Loading

Relationship to PathGlobs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Improve performance of `Owners` rule #16563

Improve performance of `Owners` rule #16563

Eric-Arellano commented Aug 17, 2022 •

edited

Loading

Relationship to `PathGlobs`