Skip to content
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

Pool analyzer diagnostic reporters #39034

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@agocke
Copy link
Contributor

commented Oct 3, 2019

This has shown up as a significant source of allocations. The design
here is that when an analyzer reports a diagnostic we may need to filter
or alter it based on some configuration. Previously we stored the
configuration information by closing over captured variables, but that
could be very expensive if it's being done for every analyzer
invocation. Since we can only really execute one analyzer per running
thread on the machine, the actual number of concurrent objects we need
to create is approximately the number of running threads. Pooling is a
good solution to this problem.

Before:


|                      Method |    Mean |   Error |  StdDev |  Median |     Min |     Max |       Gen 0 |      Gen 1 |     Gen 2 | Allocated |
|---------------------------- |--------:|--------:|--------:|--------:|--------:|--------:|------------:|-----------:|----------:|----------:|
| GetDiagnosticsWithAnalyzers | 16.28 s | 0.149 s | 0.268 s | 16.23 s | 15.76 s | 16.97 s | 200000.0000 | 59000.0000 | 2000.0000 |   1.48 GB |

After:

|                      Method |    Mean |   Error |  StdDev |  Median |     Min |     Max |       Gen 0 |      Gen 1 |     Gen 2 | Allocated |
|---------------------------- |--------:|--------:|--------:|--------:|--------:|--------:|------------:|-----------:|----------:|----------:|
| GetDiagnosticsWithAnalyzers | 15.77 s | 0.176 s | 0.519 s | 15.67 s | 14.96 s | 17.07 s | 184000.0000 | 55000.0000 | 2000.0000 |   1.36 GB |

Note: I don't think this is that much faster -- my machine wasn't completely quiet during the noise measurements, but I do believe that bytes allocated is accurate and a significant improvement.

@agocke agocke force-pushed the agocke:pool-diagnostic-reporters branch 2 times, most recently from 1974cad to fc4bf87 Oct 5, 2019
@agocke agocke changed the title WIP Pool analyzer diagnostic reporters Oct 8, 2019
@agocke agocke marked this pull request as ready for review Oct 8, 2019
@agocke agocke requested a review from dotnet/roslyn-compiler as a code owner Oct 8, 2019
@agocke agocke force-pushed the agocke:pool-diagnostic-reporters branch 2 times, most recently from 80bb300 to bf2563a Oct 8, 2019
This has shown up as a significant source of allocations. The design
here is that when an analyzer reports a diagnostic we may need to filter
or alter it based on some configuration. Previously we stored the
configuration information by closing over captured variables, but that
could be very expensive if it's being done for every analyzer
invocation. Since we can only really execute one analyzer per running
thread on the machine, the actual number of concurrent objects we need
to create is approximately the number of running threads. Pooling is a
good solution to this problem.
@agocke agocke force-pushed the agocke:pool-diagnostic-reporters branch from bf2563a to f34b4a7 Oct 8, 2019
Copy link
Contributor

left a comment

awesome!

@RikkiGibson RikkiGibson requested a review from dotnet/roslyn-compiler Oct 8, 2019
Copy link
Member

left a comment

:shipit:

@agocke agocke merged commit e2879fc into dotnet:master Oct 9, 2019
18 checks passed
18 checks passed
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI Build #20191008.31 succeeded
Details
roslyn-CI (Linux_Test coreclr) Linux_Test coreclr succeeded
Details
roslyn-CI (SourceBuild_Test) SourceBuild_Test succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests debug) Windows_CoreClr_Unit_Tests debug succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests release) Windows_CoreClr_Unit_Tests release succeeded
Details
roslyn-CI (Windows_Correctness_Test) Windows_Correctness_Test succeeded
Details
roslyn-CI (Windows_Desktop_Spanish_Unit_Tests) Windows_Desktop_Spanish_Unit_Tests succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_32) Windows_Desktop_Unit_Tests debug_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_64) Windows_Desktop_Unit_Tests debug_64 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_32) Windows_Desktop_Unit_Tests release_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_64) Windows_Desktop_Unit_Tests release_64 succeeded
Details
roslyn-CI (Windows_Determinism_Test) Windows_Determinism_Test succeeded
Details
roslyn-CI (macOS_Test) macOS_Test succeeded
Details
roslyn-integration-CI Build #20191008.31 succeeded
Details
roslyn-integration-CI (VS_Integration debug_async) VS_Integration debug_async succeeded
Details
roslyn-integration-CI (VS_Integration release_async) VS_Integration release_async succeeded
Details
@agocke agocke deleted the agocke:pool-diagnostic-reporters branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.