csharpier icon indicating copy to clipboard operation
csharpier copied to clipboard

perf: use `ValueListBuilder` for `PrivatePrintLeadingTrivia`

Open TimothyMakkison opened this issue 10 months ago • 3 comments

Added ValueListBuilder to PrivatePrintLeadingTrivia saving 0.55 MB

  • Added ValueListBuilder.Pop - this is an unmodified copy of microsofts Pop
  • Added my own version of Insert(int index, T item)
  • Added ValueListBuilder.ToList this is because CompilationUnit needs the resulting IList<Doc> contents to be an actual List<Doc> instead of an array.
    • I could modify CompilationUnit.Print to not need a mutable list WDYT.

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.

TimothyMakkison avatar Mar 09 '25 23:03 TimothyMakkison

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.

belav avatar Mar 16 '25 20:03 belav

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

TimothyMakkison avatar Jul 15 '25 20:07 TimothyMakkison

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.

belav avatar Jul 27 '25 15:07 belav