Skip to content

perf: replace StringBuilder with ValueListBuilder<char> #1545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Mar 18, 2025

  • Replaces StringBuilder with ValueListBuilder<char>
  • Added some extension methods to make this easier
  • Uses SkipLocalsInit to avoid the cost to zero stackalloc char[]

Benchmarks

I have no idea why, all the new changes I make are slower in the benchmarks, even when they should be a straight upgrade 🤔
I'm running into this issue a lot with #1542

Before

Method Mean Error StdDev Median Gen0 Gen1 Allocated
Default_CodeFormatter_Tests 133.7 ms 2.65 ms 3.54 ms 133.7 ms 2000.0000 1000.0000 31.07 MB
Default_CodeFormatter_Complex 284.9 ms 5.60 ms 13.95 ms 279.1 ms 5000.0000 2000.0000 54.59 MB

After

Method Mean Error StdDev Median Gen0 Gen1 Gen2 Allocated
Default_CodeFormatter_Tests 136.2 ms 2.69 ms 5.74 ms 134.5 ms 2000.0000 1000.0000 - 30.71 MB
Default_CodeFormatter_Complex 285.6 ms 5.71 ms 14.93 ms 278.1 ms 6000.0000 3000.0000 1000.0000 53.45 MB

@belav belav force-pushed the main branch 4 times, most recently from eeaf247 to 9e9ad83 Compare July 10, 2025 15:48
@TimothyMakkison TimothyMakkison force-pushed the VLB_string_builder branch 2 times, most recently from 3ea9bb4 to b7586f4 Compare July 13, 2025 21:26
@TimothyMakkison TimothyMakkison marked this pull request as ready for review July 13, 2025 21:27
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.

Did you figure out why your old benchmarks showed this as being slightly slower?

@TimothyMakkison
Copy link
Contributor Author

TimothyMakkison commented Jul 18, 2025

In retrospect I think I was being overly paranoid. 😓 This is a net positive for memory usage with a negligible improvement to speed. The benchmarks naturally vary a little

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