Issue 662 explore globset #883

CelebrateVC · 2022-11-23T00:38:52Z

Thank you for submitting this pull request! We appreciate you spending the time to work on these changes.

What is the motivation?

In #662 Charlie was asking if globset could speed up runtimes of file matching. After discussing a test strategy for the use case was decided.

What does this change do?

swaps out glob for globset entirely, Strips code of FilePattern as those are now compiled Globs which instead of being stored in Vec<FilePattern>s are grouped together in a GlobSet. is_excluded is modified to reflect this change and now takes in GlobSets.

What is your testing strategy?

Compiled the base image using glob and compiled altered code, per Contributing guide, benchmarked using hyperfine. Based on the discussion in #662 kneecapped the linter.rs#lint_path function to immediately return default Diagnostic, in order to exclusively test path discovery. below are current code state benchmarks.

Is this related to any issues?

#662 - as above mentioned

Have you read the Contributing Guidelines?

I have read the Contributing Guidelines

Signed-off-by: Simon (Desktop) <[email protected]>

Moving forward Vec<FilePattern> or similar should be compiled into a GlobSet, still need builder function `from_user` to create patterns for both filenames and fully specified directories Signed-off-by: Simon (Desktop) <[email protected]>

everything is now considered complex, so `exclude_simple` would never trigger, ditto ditto for extend_exclude Signed-off-by: Simon (Desktop) <[email protected]>

Signed-off-by: Simon (Desktop) <[email protected]>

charliermarsh

Nice! It seems like the code both got simpler and there are meaningful performance gains here (I'm seeing a 10ms difference on the benchmark on my machine, which is a > 3% speed-up).

charliermarsh · 2022-11-23T03:10:14Z

src/settings/user.rs

                .map(|per_file_ignore| {
                    (
-                        Exclusion::from_file_pattern(per_file_ignore.pattern),
+                        Exclusion::from_file_pattern(&per_file_ignore.pattern),


I think we should remove Exclusion and just inline the string, now that basename and absolute are identical.

cc32661 re-split basename and absolute, we should continue to use Exclusions

charliermarsh · 2022-11-23T03:11:16Z

src/settings/user.rs

-                .map(Exclusion::from_file_pattern)
-                .collect(),
+            exclude: configuration.exclude,
+            extend_exclude: configuration.extend_exclude,


I think we need to map these to strings somehow -- the current output of cargo run resources/test/fixtures/ --show-settings isn't very user-friendly:

Good points all around, I'll go over everything you have shared and take your advice on improvements, this especially should absolutely be cleaned up to something readable ( all those numbers are character codes so it shouldn't be too hard to get strings back out).

Sounds good! Thank you for your contribution :)

If at any point you feel like you won't have time to get to the comments, just let me know and we can figure out a plan.

e4d537d and 9e59346 - Had to keep an uncompiled version of the globs to get the strings needed but settings now look much cleaner

charliermarsh · 2022-11-23T03:14:15Z

src/fs.rs

+        for x in ex {
+            build.add(x);
+        }
+        build.build().expect("bad")


Nit: can we add more expansive expectations here? Like .expect("Failed to build GlobSet")?

With the changes in f4660bd, this .expect is no longer needed. (or really it's now a ? to waterfall errors up)

charliermarsh · 2022-11-23T03:25:46Z

src/settings/configuration.rs

+
+        for x in ex {
+            exclude_extend.add(x?);
+        }


Maybe a little tighter as:

let extend_exclude = { let mut set = globset::GlobSetBuilder::new(); for path in options.extend_exclude.unwrap_or_default() { set.add(from_user(&path, project_root)?); } set.build()? };

f4660bd - great suggestion

charliermarsh · 2022-11-23T03:26:13Z

src/settings/configuration.rs

-        FilePattern::Simple("venv"),
-    ]
-});
+static DEFAULT_EXCLUDE: Lazy<Vec<std::result::Result<globset::Glob, globset::Error>>> =


I wonder if this should just be a GlobSet, rather than a vector of Glob?

This was a great idea, except that we need to use the globs to make settings look nice, but I did clean this up to not include Result anymore just a Lazy<Vec<Glob>>>

charliermarsh · 2022-11-23T03:26:46Z

src/main.rs

+        for x in cli
+            .exclude
+            .iter()
+            .map(|path| from_user(path, project_root.as_ref()))


I think if we're going to for-loop here, maybe just make this part of the body, rather than doing a comprehension with the loop too. What do you think?

Absolutely, the map was just an artifact of old code, fixed in 708d408 and further improved by cc32661

charliermarsh · 2022-11-23T03:27:34Z

src/settings/types.rs

-        let absolute = Pattern::new(&absolute_path.to_string_lossy())?;
-        let basename = if pattern.contains(std::path::MAIN_SEPARATOR) {
-            None
+pub fn from_user(


This name made more sense when it was part of FilePattern, since FilePattern::from_user had clear semantics. Maybe we rename to create_glob or glob_from_user_pattern?

charliermarsh · 2022-11-23T03:28:07Z

src/settings/types.rs

-    }
+            std::borrow::Cow::Owned(
+                "{".to_owned() + &absolute_path.to_string_lossy() + "," + pattern + "}",
+            )


I think this merits a comment. Is this creating a pattern that's effectively a union of the absolute and base patterns?

Would it be clearer to instead return two Globs, one for each of those two patterns?

2 Globs made much more sense to me, so I basically reimplemented FilePattern::Complex in cc32661 🤣

charliermarsh · 2022-11-23T03:36:05Z

Thank you for this PR! Let me know if you're up for tackling the comments.

Signed-off-by: Simon (Desktop) <[email protected]>

add flexibility to Exclusion to break up Multi-part globs change created for settings display purposes Signed-off-by: Simon (Desktop) <[email protected]>

these can be used to turn items into strings in UserConfigurations Signed-off-by: Simon (Desktop) <[email protected]>

Basically reimplement Complex, but keep GlobSet for faster checks Signed-off-by: Simon (Desktop) <[email protected]>

CelebrateVC · 2022-11-23T20:17:29Z

I think that's all your reviews addressed.

Benchmarks for me are still showing ~10 ms improvement even after changes. Do LMK if there are any other changes you'd like to see. I won't be on here over Thanksgiving weekend, but we can continue going back and forth on changes after that if any are needed.

charliermarsh · 2022-11-24T04:24:47Z

Great, thanks! I'll try to get this merged in the next few days.

charliermarsh · 2022-11-25T03:32:38Z

@CelebrateVC - Thank you for this! I ended up making some tweaks to restore FilePattern (rather than using a tuple with Option), and moved the GlobSet stuff out of Configuration and into Settings which (I think) simplified a few things.

CelebrateVC added 6 commits November 22, 2022 19:06

switch glob for globset

785d286

Signed-off-by: Simon (Desktop) <[email protected]>

remove concept of FilePattern

5e0f730

Moving forward Vec<FilePattern> or similar should be compiled into a GlobSet, still need builder function `from_user` to create patterns for both filenames and fully specified directories Signed-off-by: Simon (Desktop) <[email protected]>

is_excluded refactored to take GlobSets

d8c3009

everything is now considered complex, so `exclude_simple` would never trigger, ditto ditto for extend_exclude Signed-off-by: Simon (Desktop) <[email protected]>

modify fs.rs test suite to new pattern

da433f7

Signed-off-by: Simon (Desktop) <[email protected]>

Settings should compile GlobSet from specified cfg

347783a

Signed-off-by: Simon (Desktop) <[email protected]>

pedantic clippy improvements

34ad5cc

Signed-off-by: Simon (Desktop) <[email protected]>

charliermarsh reviewed Nov 23, 2022

View reviewed changes

CelebrateVC added 5 commits November 22, 2022 22:56

Tighten up configs

f4660bd

Signed-off-by: Simon (Desktop) <[email protected]>

refactor function name

708d408

Signed-off-by: Simon (Desktop) <[email protected]>

revert UserConfigurations to use Vec<Exclusions>

e4d537d

add flexibility to Exclusion to break up Multi-part globs change created for settings display purposes Signed-off-by: Simon (Desktop) <[email protected]>

Extend Configuration to include Vec<Glob>

9e59346

these can be used to turn items into strings in UserConfigurations Signed-off-by: Simon (Desktop) <[email protected]>

Refactor create_glob to sometimes return 2 Globs

cc32661

Basically reimplement Complex, but keep GlobSet for faster checks Signed-off-by: Simon (Desktop) <[email protected]>

charliermarsh added 2 commits November 24, 2022 18:11

Merge branch 'main' into pr-883

6b53354

Restore FilePattern type; remove Glob references from Configuration

52c3afe

charliermarsh merged commit a3af6c1 into astral-sh:main Nov 25, 2022

CelebrateVC deleted the issue-662-explore-globset branch December 11, 2022 17:04

Issue 662 explore globset #883

Issue 662 explore globset #883

Uh oh!

Conversation

CelebrateVC commented Nov 23, 2022

What is the motivation?

What does this change do?

What is your testing strategy?

Is this related to any issues?

Have you read the Contributing Guidelines?

Uh oh!

charliermarsh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CelebrateVC Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charliermarsh commented Nov 23, 2022

Uh oh!

CelebrateVC commented Nov 23, 2022

Uh oh!

charliermarsh commented Nov 24, 2022

Uh oh!

charliermarsh commented Nov 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CelebrateVC Nov 23, 2022 •

edited

Loading