perf: use `ValueListBuilder` for `PrivatePrintLeadingTrivia`
Added ValueListBuilder to PrivatePrintLeadingTrivia saving 0.55 MB
- Added
ValueListBuilder.Pop- this is an unmodified copy of microsoftsPop - Added my own version of
Insert(int index, T item) - Added
ValueListBuilder.ToListthis is becauseCompilationUnitneeds the resultingIList<Doc> contentsto be an actualList<Doc>instead of an array.- I could modify
CompilationUnit.Printto not need a mutable list WDYT.
- I could modify
This isn't a massive saving I can close this if you like. I suspect this is more impactful for projects with a lot of comments and summaries though.
I'm wondering if keeping it is worth it to try to keep Token consistently using ValueListBuilder.
Besides this, multiline raw strings also make use of List<Doc>, I'm not sure how much they would benefit from it.
In theory I think CompilationUnit could set things to Doc.Null instead of removing them. I think it is pretty rare that it actually ends that code block, so it may be worth it to avoid the ToList call. And some day if I can find the time I did want to try to redo all of this comment logic, which would probably also get rid of the need for ToList.
Sorry about taking forever, getting back into programming now. I've looked at this PR and it seems okay. Not sure if it's worth the effort although I'm sure some comment heavy code will benefit from this.
Did csharpier remove caching recently? CSharpier takes up to 10 seconds to run on this project when it used to take 400ms. Edit: I've briefly looked at #1588, how does waiting on gitignore cause this degradation?
Benchmarks
Note that timing is inaccurate, small memory improvements.
Current
| Method | Mean | Error | StdDev | Median | Gen0 | Gen1 | Allocated |
|---|---|---|---|---|---|---|---|
| Default_CodeFormatter_Tests | 234.1 ms | 8.78 ms | 24.92 ms | 227.8 ms | 3000.0000 | 1000.0000 | 35.15 MB |
| Default_CodeFormatter_Complex | 344.5 ms | 25.06 ms | 72.70 ms | 317.3 ms | 5000.0000 | 2000.0000 | 54.85 MB |
Changes
| Method | Mean | Error | StdDev | Median | Gen0 | Gen1 | Allocated |
|---|---|---|---|---|---|---|---|
| Default_CodeFormatter_Tests | 186.3 ms | 4.64 ms | 13.16 ms | 181.6 ms | 3000.0000 | 1000.0000 | 34.58 MB |
| Default_CodeFormatter_Complex | 396.5 ms | 11.07 ms | 31.75 ms | 395.6 ms | 5000.0000 | 2000.0000 | 54.51 MB |
I've looked at this PR and it seems okay. Not sure if it's worth the effort although I'm sure some comment heavy code will benefit from this.
One argument for this could be moving towards using ValueListBuilder consistently. If there are some benefits the code isn't that complex. And if that code is used everywhere it becomes the normal way of writing the formatting code.
On the other hand it could be a lot of work to try to use it everywhere.
Did csharpier remove caching recently? CSharpier takes up to 10 seconds to run on this project when it used to take 400ms.
I haven't had any significant performance issues. I'm at around 2.6s with no caching and 1.4s with caching. I do know that the new xml formatting added some overhead.
Edit: I've briefly looked at https://github.com/belav/csharpier/issues/1588, how does waiting on gitignore cause this degradation?
#1595 in theory should have minimal overhead and makes sure no two threads are duplicating the work of parsing the gitignore. I haven't seen any degradation from it.