-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove allocations in ErorrFacts #75234
base: main
Are you sure you want to change the base?
Conversation
When profiling our unit tests I discovered that `ErrorFacts` are a significant source of `string` allocations. In `Emit` for example it can be over 200MB of allocations. This is due to our use of `Enum.ToString` as well as concat that value with well known constants. To avoid these allocations I changed our code generator to produce these strings at compile time. I was a bit concerned about the performance of the `switch` compared to the existing code so I wrote a quick benchmark to demonstrate that there is no perf lost here. | Method | Runtime | Mean | Ratio | Gen0 | Allocated | Alloc Ratio | |------------------- |--------------------- |----------:|------:|--------:|----------:|------------:| | EnumToString | .NET 8.0 | 66.80 us | 1.00 | - | 45046 B | 1.00 | | EnumToStringHelper | .NET 8.0 | 15.76 us | 0.24 | - | - | 0.00 | | EnumToString | .NET Framework 4.7.2 | 512.30 us | 7.69 | 21.4844 | 140197 B | 3.11 | | EnumToStringHelper | .NET Framework 4.7.2 | 17.70 us | 0.27 | - | - | 0.00 |
@dotnet/roslyn-compiler PTAL |
@@ -8129,7 +8129,7 @@ void binaryOperator(string op, string leftType, string rightType, string expecte | |||
|
|||
if (expectedDiagnostics.Length == 0) | |||
{ | |||
CompileAndVerify(comp, verify: Verification.FailsPEVerify); | |||
CompileAndVerify(comp, verify: Verification.FailsPEVerify, emitOptions: EmitOptions.Default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a different perf fix I was working on that I accidentally included with this change. I'll revert this part
I am not sure what is the common pattern for using this API, but it looks like our resources contain only 20 ids like that and all of them are for warnings. Refers to: src/Compilers/CSharp/Portable/Generated/ErrorFacts.Generated.cs:4157 in cbcb2e5. [](commit_id = cbcb2e5, deletion_comment = False) |
Do we understand why tests are requesting the strings? Would it be possible to decrease the allocation just by making changes to the test infrastructure? |
If we go with the change, we probably should adjust the following comment at the end of
|
I am not sure that we are optimizing for a common real-world scenario, but we are definitely paying with more memory usage by the compiler (string heap, code size, etc.) for all scenarios. |
The primary method that causes issues is
The places I've dug into need to realize the string in some way to do their job.
I agree that is a consideration we should make. The The |
I dug into this a bit more and wrote a program to dump the
Looking at that I agree that the price we're paying for |
When profiling our unit tests I discovered that
ErrorFacts
are a significant source ofstring
allocations. InEmit
for example it can be over 200MB of allocations.This is due to our use of
Enum.ToString
as well as concat that value with well known constants. To avoid these allocations I changed our code generator to produce these strings at compile time.I was a bit concerned about the performance of the
switch
compared to the existing code so I wrote a quick benchmark to demonstrate that there is no perf lost here.