Reduce Linq usage in FileSystemGlobbing #98109

eerhardt · 2024-02-07T16:25:51Z

Since GetResultsInFullPath uses a struct (FilePatternMatch) in a Linq call, this is causing extra disk space on native AOT'd applications because of struct specialization. Remove the Linq call and instead use a simple loop to create the result.

Using the following program:

using Microsoft.Extensions.FileSystemGlobbing;

var matcher = new Matcher();
matcher.AddIncludePatterns(["*.txt", "*.asciidoc", "*.md"]);

string searchDirectory = @"C:\temp";
foreach (string file in matcher.GetResultsInFullPath(searchDirectory))
{
    Console.WriteLine(file);
}

win-x64 size on disk:

Before: 1.59 MB (1,675,776 bytes)
After: 1.56 MB (1,642,496 bytes)

The remaining System.Linq usages in the library don't seem beneficial to remove:

runtime/src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/InMemoryDirectoryInfo.cs

Line 48 in 47f325e

var fileList = new List<string>(files.Count());

runtime/src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/PatternMatchingResult.cs

Line 20 in 47f325e

: this(files, hasMatches: files.Any())

Since GetResultsInFullPath uses a struct (FilePatternMatch) in a Linq call, this is causing extra disk space on native AOT'd applications because of struct specialization. Remove the Linq call and instead use a simple loop to create the result.

ghost · 2024-02-07T16:26:07Z

Tagging subscribers to this area: @dotnet/area-extensions-filesystem
See info in area-owners.md if you want to be subscribed.

Issue Details

Since GetResultsInFullPath uses a struct (FilePatternMatch) in a Linq call, this is causing extra disk space on native AOT'd applications because of struct specialization. Remove the Linq call and instead use a simple loop to create the result.

Using the following program:

using Microsoft.Extensions.FileSystemGlobbing;

var matcher = new Matcher();
matcher.AddIncludePatterns(["*.txt", "*.asciidoc", "*.md"]);

string searchDirectory = @"C:\temp";
foreach (string file in matcher.GetResultsInFullPath(searchDirectory))
{
    Console.WriteLine(file);
}

win-x64 size on disk:

Before: 1.59 MB (1,675,776 bytes)
After: 1.56 MB (1,642,496 bytes)

The remaining System.Linq usages in the library don't seem beneficial to remove:

runtime/src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/InMemoryDirectoryInfo.cs

Line 48 in 47f325e

var fileList = new List<string>(files.Count());

runtime/src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/PatternMatchingResult.cs

Line 20 in 47f325e

: this(files, hasMatches: files.Any())

Author:	eerhardt
Assignees:	-
Labels:	`area-Extensions-FileSystem`
Milestone:	-

eerhardt · 2024-02-07T16:28:01Z

src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/MatcherExtensions.cs

+            IEnumerable<FilePatternMatch> matches = patternMatchingResult.Files;
+            List<string> result = matches is ICollection matchCollection ? new(matchCollection.Count) : new();
+            foreach (FilePatternMatch match in matches)
+            {
+                result.Add(Path.GetFullPath(Path.Combine(directoryPath, match.Path)));
+            }
            return result;


I'm not 100% confident this is better. But it does reduce the size on disk. I'm assuming it also reduces JIT cost and in-memory code size as well since we no longer need to specialize the Linq code for this struct.

Thoughts?

Why is the ToArray/ToList needed at all? Are we expecting consumers to iterate it multiple times?

Why is the ToArray/ToList needed at all?

I was trying to keep the behavior as close to the existing as possible. I assume you are suggesting possibly just using a yield return enumerator instead?

Are we expecting consumers to iterate it multiple times?

I don't think multiple times, but a common pattern I see (a little less than 50% from doing a few sample checks) is to call .ToList() or .ToImmutableArray() on the result of this method call. Having the result of this method return something that implements ICollection/Count seems to be beneficial in those cases.

examples:

https://github.com/dotnet/format/blob/e07ea4767a72b0f81457599e92d9a89ae9cf18bd/src/Workspaces/FolderWorkspace_FolderSolutionLoader.cs#L54

https://github.com/morganstanley/dotnet-please/blob/d9c78451823fc2109a7b27664748b3159d35d64a/DotNetPlease/Helpers/MSBuildHelper.cs#L131-L144

I assume you are suggesting possibly just using a yield return enumerator instead?

Or just returning the Select, in which case someone calling ToList on it will pick up Select's implementation of it. I'm not sure which aspect of your change was the focus and whether your explicit goal was to remove the Select.

I'm not sure which aspect of your change was the focus and whether your explicit goal was to remove the Select.

Yes, it was to remove the Select over a struct (in this case FilePatternMatch). The main goal was to reduce the generic specialization code generated in NAOT.

stephentoub · 2024-03-07T15:05:44Z

src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/MatcherExtensions.cs

+            }

+            IEnumerable<FilePatternMatch> matches = patternMatchingResult.Files;
+            List<string> result = matches is ICollection matchCollection ? new(matchCollection.Count) : new();


matches is ICollection

ICollection<T> doesn't inherit ICollection; many of our collection types implement both, but it's not guaranteed. Is this just a "good enough" thing because we expect most inputs would be e.g. T[] or List<T>?

Judging by the source, matches will always be a List<T> here.

@eerhardt is my understanding correct? If so, to reduce complexity a bit, can we remove the new() fallback and maybe add an "is ICollection" assertion?

Will merge the change without this since its still valuable.

Sorry I missed the original question.

Since this is a public API and Matcher.Execute is virtual, the PatternMatchingResult can have any IEnumerable<FilePatternMatch> Files set on it. It isn't guaranteed to always be a List<T>.

Is this just a "good enough" thing because we expect most inputs would be e.g. T[] or List?

Yes, our implementation always returns a List<T>, so we typically expect this to be List<T> in most cases.

* Reduce Linq usage in FileSystemGlobbing Since GetResultsInFullPath uses a struct (FilePatternMatch) in a Linq call, this is causing extra disk space on native AOT'd applications because of struct specialization. Remove the Linq call and instead use a simple loop to create the result. * Use ToArray instead of ToList to reduce unnecessary allocations. --------- Co-authored-by: David Cantú <[email protected]>

eerhardt added 2 commits February 6, 2024 18:45

Reduce Linq usage in FileSystemGlobbing

b798440

Since GetResultsInFullPath uses a struct (FilePatternMatch) in a Linq call, this is causing extra disk space on native AOT'd applications because of struct specialization. Remove the Linq call and instead use a simple loop to create the result.

Use ToArray instead of ToList to reduce unnecessary allocations.

bd9e2a3

eerhardt requested a review from stephentoub February 7, 2024 16:25

ghost added the area-Extensions-FileSystem label Feb 7, 2024

ghost assigned eerhardt Feb 7, 2024

eerhardt commented Feb 7, 2024

View reviewed changes

stephentoub mentioned this pull request Mar 5, 2024

Consolidate LINQ's internal IIListProvider/IPartition into base Iterator class #98969

Merged

stephentoub reviewed Mar 7, 2024

View reviewed changes

stephentoub approved these changes Mar 7, 2024

View reviewed changes

jozkee approved these changes Mar 15, 2024

View reviewed changes

Merge branch 'main' into ReduceLinqInFileSystemGlobbing

d738046

jozkee merged commit 3725e24 into dotnet:main Apr 19, 2024

This was referenced Apr 20, 2024

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

Timeout in System.Text.Json.Tests.Utf8JsonWriterTests.WriteNumbers #101193

Closed

eerhardt deleted the ReduceLinqInFileSystemGlobbing branch April 20, 2024 21:55

github-actions bot locked and limited conversation to collaborators May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reduce Linq usage in FileSystemGlobbing #98109

Reduce Linq usage in FileSystemGlobbing #98109

Uh oh!

eerhardt commented Feb 7, 2024

Uh oh!

ghost commented Feb 7, 2024

Uh oh!

eerhardt Feb 7, 2024

Uh oh!

stephentoub Feb 9, 2024

Uh oh!

eerhardt Feb 9, 2024

Uh oh!

stephentoub Feb 9, 2024

Uh oh!

eerhardt Feb 9, 2024

Uh oh!

stephentoub Mar 7, 2024

Uh oh!

jozkee Mar 15, 2024

Uh oh!

jozkee Mar 18, 2024

Uh oh!

jozkee Apr 19, 2024

Uh oh!

eerhardt Apr 20, 2024

Uh oh!

eerhardt Apr 20, 2024

Uh oh!

Uh oh!

Reduce Linq usage in FileSystemGlobbing #98109

Reduce Linq usage in FileSystemGlobbing #98109

Uh oh!

Conversation

eerhardt commented Feb 7, 2024

Uh oh!

ghost commented Feb 7, 2024

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!