Skip to content

Conversation

@TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Mar 7, 2025

Follow on from #1502, using ValueListBuilder to replace known List<Doc> sizes.

  • Most of these changes aren't measurable in the benchmarks, many are insignificant, although I suspect this is partly due to the limited code found in the benchmark file.
  • This PR will increase the performance benefits of perf: add new Concat types for children lengths #1524

@belav what do you think the best way of replacing a list that uses a collection initializer is?

I can either add the items directly into the scratch buffer and manually update Length

var docs = new ValueListBuilder<Doc>
([
    Modifiers.Print(node.Modifiers, context),
    Token.Print(node.DelegateKeyword, context),
    null,null
]);
docs.Length = 2;

Or create the buffer and then append the items via params.

var docs = new ValueListBuilder<Doc>([null, null, null, null]);
docs.Append(
    Modifiers.Print(node.Modifiers, context),
    Token.Print(node.DelegateKeyword, context)
);

Or Append each item

var docs = new ValueListBuilder<Doc>([null, null, null, null]);
docs.Append(Modifiers.Print(node.Modifiers, context));
docs.Append(Token.Print(node.DelegateKeyword, context));

@TimothyMakkison TimothyMakkison force-pushed the syntax_print_vlb branch 4 times, most recently from cdc306a to a706675 Compare March 14, 2025 17:40
@TimothyMakkison TimothyMakkison force-pushed the syntax_print_vlb branch 3 times, most recently from a2ae394 to e5e79b2 Compare March 17, 2025 23:20
@TimothyMakkison TimothyMakkison marked this pull request as ready for review March 17, 2025 23:20
@TimothyMakkison
Copy link
Contributor Author

TimothyMakkison commented Mar 17, 2025

  • Opted to append each item to keep similar formatting
  • Double checked the scratch buffer sizes for each method
  • Added Doc.Concat(ref ValueListBuilder<Doc>) for the token and separated syntax list methods. It might be worth adding AggressiveInlining to it 🤔

Nice little performance gain and saves 1.5 MB on the complex code benchmark, it used to be larger before #1535

I'll look into doing this for the variable size buffers later.

Before

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
Default_CodeFormatter_Tests 102.3 ms 2.04 ms 1.91 ms 3333.3333 2333.3333 666.6667 31.13 MB
Default_CodeFormatter_Complex 173.5 ms 2.52 ms 3.99 ms 5000.0000 2000.0000 - 54.57 MB

After

Method Mean Error StdDev Median Gen0 Gen1 Gen2 Allocated
Default_CodeFormatter_Tests 95.02 ms 1.878 ms 3.793 ms 93.56 ms 3000.0000 1666.6667 333.3333 30.81 MB
Default_CodeFormatter_Complex 168.67 ms 1.949 ms 2.795 ms 168.72 ms 5000.0000 2000.0000 - 53.08 MB

When combined with #1524 (timing is inaccurate)

Method Mean Error StdDev Median Gen0 Gen1 Allocated
Default_CodeFormatter_Tests 127.0 ms 2.33 ms 3.49 ms 127.3 ms 2000.0000 1000.0000 29.21 MB
Default_CodeFormatter_Complex 264.6 ms 6.40 ms 18.88 ms 271.5 ms 4000.0000 2000.0000 49.97 MB

belav
belav previously approved these changes Apr 4, 2025
Copy link
Owner

@belav belav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opted to append each item to keep similar formatting

That is the option I would have gone for. I think it keeps it cleaner. I also like consolidating the logic for dealing with Concat with 0, 1 or more items. Nice work!

I knew doing this in gh was a bad idea.
@belav belav merged commit 5a9e32d into belav:main Apr 4, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants