Improved memory performance of TextColorToGenerator

[AllenD-MSFT]

Description of Change

The LINQ Single method is subject to boxing and poor memory allocation behavior. For hot paths like this it is better to avoid boxing enumerators of built-in types and also to better to unroll LINQ queries to avoid allocation behavior that causes GC pressure.

This proposed code has the same exception characteristics as .Single so callers relying on any such behavior are unaffected.

Testing memory allocation and/or boxing is not a recommended practice so no new tests are added. Existing tests should suffice. Sample code is also not needed since this is just existing method maintenance with no change in expected behavior.

Linked Issues

Pending: Still working out the process for this change relative to issue(s).

• Fixes #

PR Checklist

• Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
• Has tests (if omitted, state reason in description)
• Has samples (if omitted, state reason in description)
• Rebased on top of main at time of PR
• Changes adhere to coding standard
• Documentation created or updated: No need for a documentation change.

[brminnick]

Cool - thanks @AllenD-MSFT! Do you happen to have any benchmarks on this PR that we can use to promote this performance improvement?

[AllenD-MSFT]

Cool - thanks @AllenD-MSFT! Do you happen to have any benchmarks on this PR that we can use to promote this performance improvement?

I don't. Are there instructions on generating benchmarks?

This is my first time contributing so I'm not fully versed on all the ways to work here, but happy to learn.

[brminnick]

No worries! I typically recommend using Benchmark .NET to test the before-and-after performance.

I haven't tried it, but it looks like you can also use Microsoft.Diagnostics.Tracing.TraceEvent to test performance on Source Generators: https://www.meziantou.net/measuring-performance-of-roslyn-source-generators.htm

Long story short, we just want to confirm that this PR does provide a performance improvement before merging it.

[AllenD-MSFT]

No worries! I typically recommend using Benchmark .NET to test the before-and-after performance.

I haven't tried it, but it looks like you can also use Microsoft.Diagnostics.Tracing.TraceEvent to test performance on Source Generators: https://www.meziantou.net/measuring-performance-of-roslyn-source-generators.htm

Long story short, we just want to confirm that this PR does provide a performance improvement before merging it.

Thanks. I'll give Benchmark .NET a try.

[pictos]

@AllenD-MSFT I would say that following the instructions on link that Brandon shared will be enough, you can just share a before/after screenshot from the binlog window. Let me know if you need any help on that

[brminnick]

Hey @AllenD-MSFT! Any luck with the benchmarks?

[brminnick]

Hi Allen! I'm closing this PR for now. I'm happy to re-open + merge it once we have the benchmark data.

[davkean]

@brminnick I see this got closed. I work on the Visual Studio perf team and this continues to show up. Looking at this sort of thing all day, I can see just by reading the code that it will perform much better than the prior code.

The current code in its worst case that was caught in the wild allocates at 229 MB/second due to it the capture class, delegate and boxed enumerator allocations. Requiring folks to prove that unwrapping LINQ into a foreach allocates less and is faster seems like a high burden of proof that not even Microsoft requires on their own repositories, please reconsider merging this.

[brminnick]

Thanks for the review @davkean! Always good to hear from you!!

I'm happy to merge it with your stamp of approval 💯

[AllenD-MSFT]

I changed roles right after putting this up. I devised a few benchmarks, but they weren't real-world enough to show the improvement. Unit tests are like that. I'm glad @davkean noticed and jumped in.