Improve performance of CLI when passing in files/folders - #1893
Conversation
|
I finally got around to refactoring CommandLineFormatter this weekend which caused conflicts with the changes you made. Some of the logic got moved into FormattingEngine. Could you resolve the conflicts and then I can take a look? As for tests, I don't know that it is possible to test the performance, but if there aren't already some tests that pass in multiple file paths that would be good to add. |
ea9195c to
1f70f0d
Compare
|
I have rebased the change onto latest in main. The change is a lot more spread out after the cli refactor. The biggest complicating factor is still the FormattingEngine now has a method called CommandLineFormatter now checks if file paths exist and if msbuild versions are correct before starting formatting. It also only creates one OptionsProvider, FormattingCache, and FormattingEngine per cli run instead of one per file/folder. I also added some tests for having multiple editorconfigs, testing the ignore-path option with multiple root directories passed in. FormattingCache now uses PrinterOptions hash instead of OptionsProvider since PrinterOptions defines all the evaluated configurations (indent, line endings, xml spacing, ...) and doesn't include extra information such as ignore file or directory mappings that don't effect how files are formatted. |
- Added `CliBenchmarks` containing benchmarks for formatting a directory and checking a list of files. - Uses the `CSharpier` project to format and check. This is more of a benchmark to measure the time to get the files as opposed to a formatting benchmark as the files will become cached. - As Soda found in #1893, `CSharpier` is very bad at handing multiple files, each iteration of `CliBench` took 50 seconds for me 😑 - `Benchmarks` is getting cluttered so I split it up into `CSharpBenchmarks` and `XmlBenchmarks` Relevant to #1893 Co-authored-by: Bela VanderVoort <twobitbela@gmail.com>
# Conflicts: # Src/CSharpier.Cli/FormattingCache.cs
|
My recent changes caused some conflicts again. I got those squared away. I think there are a few potential problems but I don't have time to really dig into this right now. But the initial benchmarks are looking great! This is doing a check on ~250 passed in files # before
| Method | Mean | Error | StdDev | Gen0 | Gen1 | Allocated |
|----------- |--------:|--------:|--------:|-----------:|-----------:|----------:|
| CheckFiles | 14.30 s | 0.162 s | 0.144 s | 26000.0000 | 21000.0000 | 420.32 MB |
# after
| Method | Mean | Error | StdDev | Gen0 | Gen1 | Gen2 | Allocated |
|----------- |---------:|---------:|---------:|----------:|----------:|----------:|----------:|
| CheckFiles | 354.7 ms | 16.95 ms | 49.97 ms | 7000.0000 | 5000.0000 | 1000.0000 | 102.75 MB | |
Only create one instance of OptionsProvider and FormattingCache per CLI run instead of creating one per file that is passed in.
Description
This PR improves the performance of running csharpier on the command line when passing in a list of files. When using a pre-commit hook it is common to pass in the list of staged files to csharpier to have the files formatted before committing. Currently this process is very slow when a large number of files are passed in. For example when testing this on the csharpier repo if every file was passed in on the command line it would take 45seconds to format. This slowdown is mainly caused by each file or folder creating its own instance of OptionsProvider which has to recalculate ignore file and config file per file.
To improve the performance this PR moves the OptionsProvider and FormattingCache outside of the file/directory loop then adds each file to a
List<Task>that gets run usingTask.WhenAll(). The one exception to this is when using--ignore-pathsince it needs a new OptionsProvider for every single directory that is passed in since when parsing the ignore file from--ignore-pathcli option it is based off the location of the directory not the ignore file itself. This is unlike prettier's--ignore-pathor git .gitignore whose ignore file are always based off the ignore file location and not the location of passed in file or directory. I can open another PR to change--ignore-pathto always base file paths from the location of the ignore file instead to simply this logic and improve performance even when using--ignore-path.Some benchmarks when testing this using cli to format all files in this repo by passing them as cli arguments.
If you think of a good way to test this I can add some tests. I was thinking of including a benchmark that used all cs files in the repo itself or creating a bunch of test cs files and using them.
Related Issue
Checklist
varthis.