Disable EventSource generator in design-time builds by stephentoub · Pull Request #50741 · dotnet/runtime

stephentoub · 2021-04-05T19:39:23Z

I think we can get away with this for the EventSource generator, as the values it generates shouldn't be needed while working in the project. But I expect we won't be as lucky with the DllImportGenerator cc: @jkoritzinsky, @AaronRobinsonMSFT. @chsienki, is there a recommendation for how to avoid the impact here for such a generator? My understanding is you're working on a replacement set of APIs, but that we'll need to switch over to using them wholesale in order to get the benefits? Do you know when they'll be available?

ghost · 2021-04-05T19:39:29Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

sharwell

This is the approach that works for <Analyzer> references; someone with project system experience should verify that the same approach using <ProjectReference> would not be problematic.

stephentoub · 2021-04-06T02:35:51Z

I'm going to merge this. Typing in VS while working in Corelib.csproj is currently painful.

AaronRobinsonMSFT · 2021-04-06T02:55:27Z

@stephentoub This is unfortunate. I hope there are simply issues with the source generator impl rather than the overall design.

@elinor-fung and @jkoritzinsky I'm going to add an IDE integration validation step to #43060.

sharwell · 2021-04-06T05:07:47Z

@AaronRobinsonMSFT in the SG V1 API, it is very difficult to avoid observable typing lag (read: no known solution exists) when both of the following conditions are met, regardless of how fast the source generator itself is:

The project is large
The source generator uses a syntax receiver

I suggested @stephentoub be added to the working group for the SG V2 API, as I do not believe the P/Invoke source generator will be possible to sidestep either of the above, and the workaround in this PR is likely to not work.

stephentoub · 2021-04-06T13:16:18Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good. cc: @ericstj

@chsienki, when is the new API that addresses this going to be ready to consume? In time to rewrite in-box source generators to use it in .NET 6? Is there an issue tracking it?

…shim_mono # By Aaron Robinson (10) and others # Via GitHub * upstream/main: (108 commits) [mbr] Add Apple sample (dotnet#50740) make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763) Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622) [mono] More domain cleanups (dotnet#50479) Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754) Disable EventSource generator in design-time builds (dotnet#50741) Fix X509 test failures on Android (dotnet#50301) Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703) Enforce 64KB event payload size limit on EventPipe (dotnet#50600) Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906) [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458) improve connection scavenge logic by doing zero-byte read (dotnet#50545) Resolve call mdtokens when making tier 1 inline observations (dotnet#50675) Annotate APIs in System.Private.Xml (dotnet#49682) Support compiling against OpenSSL 3 headers Change Configuration.Json to use a regular Dictionary. (dotnet#50611) Remove unused BigNumFromBinary P/Invoke (dotnet#50670) Make Ninja the default CMake generator on Windows for the repo (dotnet#49715) [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637) [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547) ... # Conflicts: # src/mono/dlls/mscordbi/CMakeLists.txt

AaronRobinsonMSFT · 2021-04-06T16:27:44Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good.

Thankfully the Interop source generator will not be shipping publicly for .NET 6 - only for product build. However, we will want to ensure the product development loop isn't negative profoundly impacted. It sounds like a post-.NET 6 solution is likely so we will be able to consume that prior to an official beta for non-runtime consumption.

chsienki · 2021-04-06T17:41:56Z

@stephentoub We're targeting a preview for 16.10, but it'll require the user to opt-in until the APIs reach stable.

The issue tracking it is here dotnet/roslyn#51257 although it's not very well defined to be honest (I'll try and go through and create some more granular issues that link to that one for tracking).

Unsure if the timing is going to work for rewriting the inbox generators for the .NET 6 timeline, although we'd obviously like to do so.

jaredpar · 2021-04-06T21:59:31Z

Anyone have a good explanation of

The actual perf problem that was hit here?
Why they feel the new APIs are going to address that problem?

sharwell · 2021-04-06T22:06:47Z

The actual perf problem that was hit here?

The generator driver is allocating 14GB/min in GeneratorDriver.RunGenerators because we don't have any way to incrementally update the result for a source generator that uses a syntax receiver. The vast majority of these allocations are deserializing trees that moved to temporary storage since the whole solution doesn't fit in address space at the same time.

Why they feel the new APIs are going to address that problem?

The new pipeline API will allow an O(solution size) algorithm to reduce to an O(document size) algorithm for syntax receivers that have document granularity. In addition, the document needed for incremental update is much more likely to already be in memory because it's the document currently open in the editor.

jaredpar · 2021-04-06T23:08:33Z

There are plenty of syntax receiver based generators out there that do not exhibit this problem. The receiver here is doing little more than walking the syntax tree which is done many, many times across Roslyn in the IDE. Can you elaborate more on what pattern this generator is using that is causing this behavior?

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality. And yes moving to the new APIs would "fix" the problem because it avoids the work. At the same time my suspicion is that is more masking the existing problem vs. fixing it.

sharwell · 2021-04-06T23:15:07Z

Can you elaborate more on what pattern this generator is using that is causing this behavior?

It's not walking a syntax tree, it's walking every syntax tree. In a large solution like this, the trees don't all fit in process address space at the same time, so a full walk across all the trees will cause many to get moved to temporary storage and others to be read back in. In a project small enough to hold all trees in the process address space without Gen 2 GC forcing them out, most of the allocations go away.

benaadams · 2021-04-06T23:17:04Z

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality.

Only does a name equality check if the class has an attribute with length 32 or length 23; which should be infrequent?

runtime/src/libraries/System.Private.CoreLib/generators/EventSourceGenerator.cs

Lines 69 to 83 in b3d4eb2

// Only clasess
if(syntaxNodeisClassDeclarationSyntaxclassDeclaration)
{
// Check if has EventSource attribute before adding to candidates
// as we don't want to add every class in the project
foreach(AttributeListSyntax?calinclassDeclaration.AttributeLists)
{
foreach(AttributeSyntax?caincal.Attributes)
{
// Check if Span length matches before allocating the string to check more
intlength=ca.Name.Span.Length;
if(length!=EventSourceAttribute.Length&&length!=EventSourceAttributeShort.Length)
{
continue;
}

jaredpar · 2021-04-06T23:23:26Z

Right so this is not a problem with the generator itself but more than the IDE hasn't yet move the generator infrastructure out of process. Hence you're still limited to the 32 bit address space in VS

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

sharwell · 2021-04-06T23:28:35Z

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

It's not pulling them all in at the same time. It's pulling them in as part of a background update, but it leads to significant churn. Items loaded early in one pass are released by the time the pass ends, which means they need to be reloaded during the next pass. When they are reloaded in the next pass, items at the end of the previous pass are released to make room, and those items then need to be reloaded.

The overhead could be reduced if ISyntaxReceiver had an equivalent that only needed the SyntaxTree, since the generator could avoid realizing the syntax tree if the text hash didn't change.

stephentoub · 2021-04-05T19:39:23Z

cc: @sharwell, @chsienki, @benaadams

I think we can get away with this for the EventSource generator, as the values it generates shouldn't be needed while working in the project. But I expect we won't be as lucky with the DllImportGenerator cc: @jkoritzinsky, @AaronRobinsonMSFT. @chsienki, is there a recommendation for how to avoid the impact here for such a generator? My understanding is you're working on a replacement set of APIs, but that we'll need to switch over to using them wholesale in order to get the benefits? Do you know when they'll be available?

ghost · 2021-04-05T19:39:29Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

sharwell

This is the approach that works for <Analyzer> references; someone with project system experience should verify that the same approach using <ProjectReference> would not be problematic.

stephentoub · 2021-04-06T02:35:51Z

I'm going to merge this. Typing in VS while working in Corelib.csproj is currently painful.

AaronRobinsonMSFT · 2021-04-06T02:55:27Z

@stephentoub This is unfortunate. I hope there are simply issues with the source generator impl rather than the overall design.

@elinor-fung and @jkoritzinsky I'm going to add an IDE integration validation step to #43060.

sharwell · 2021-04-06T05:07:47Z

@AaronRobinsonMSFT in the SG V1 API, it is very difficult to avoid observable typing lag (read: no known solution exists) when both of the following conditions are met, regardless of how fast the source generator itself is:

The project is large
The source generator uses a syntax receiver

I suggested @stephentoub be added to the working group for the SG V2 API, as I do not believe the P/Invoke source generator will be possible to sidestep either of the above, and the workaround in this PR is likely to not work.

stephentoub · 2021-04-06T13:16:18Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good. cc: @ericstj

@chsienki, when is the new API that addresses this going to be ready to consume? In time to rewrite in-box source generators to use it in .NET 6? Is there an issue tracking it?

…shim_mono # By Aaron Robinson (10) and others # Via GitHub * upstream/main: (108 commits) [mbr] Add Apple sample (dotnet#50740) make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763) Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622) [mono] More domain cleanups (dotnet#50479) Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754) Disable EventSource generator in design-time builds (dotnet#50741) Fix X509 test failures on Android (dotnet#50301) Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703) Enforce 64KB event payload size limit on EventPipe (dotnet#50600) Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906) [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458) improve connection scavenge logic by doing zero-byte read (dotnet#50545) Resolve call mdtokens when making tier 1 inline observations (dotnet#50675) Annotate APIs in System.Private.Xml (dotnet#49682) Support compiling against OpenSSL 3 headers Change Configuration.Json to use a regular Dictionary. (dotnet#50611) Remove unused BigNumFromBinary P/Invoke (dotnet#50670) Make Ninja the default CMake generator on Windows for the repo (dotnet#49715) [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637) [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547) ... # Conflicts: # src/mono/dlls/mscordbi/CMakeLists.txt

AaronRobinsonMSFT · 2021-04-06T16:27:44Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good.

Thankfully the Interop source generator will not be shipping publicly for .NET 6 - only for product build. However, we will want to ensure the product development loop isn't negative profoundly impacted. It sounds like a post-.NET 6 solution is likely so we will be able to consume that prior to an official beta for non-runtime consumption.

chsienki · 2021-04-06T17:41:56Z

@stephentoub We're targeting a preview for 16.10, but it'll require the user to opt-in until the APIs reach stable.

The issue tracking it is here dotnet/roslyn#51257 although it's not very well defined to be honest (I'll try and go through and create some more granular issues that link to that one for tracking).

Unsure if the timing is going to work for rewriting the inbox generators for the .NET 6 timeline, although we'd obviously like to do so.

jaredpar · 2021-04-06T21:59:31Z

Anyone have a good explanation of

The actual perf problem that was hit here?
Why they feel the new APIs are going to address that problem?

sharwell · 2021-04-06T22:06:47Z

The actual perf problem that was hit here?

The generator driver is allocating 14GB/min in GeneratorDriver.RunGenerators because we don't have any way to incrementally update the result for a source generator that uses a syntax receiver. The vast majority of these allocations are deserializing trees that moved to temporary storage since the whole solution doesn't fit in address space at the same time.

Why they feel the new APIs are going to address that problem?

The new pipeline API will allow an O(solution size) algorithm to reduce to an O(document size) algorithm for syntax receivers that have document granularity. In addition, the document needed for incremental update is much more likely to already be in memory because it's the document currently open in the editor.

jaredpar · 2021-04-06T23:08:33Z

There are plenty of syntax receiver based generators out there that do not exhibit this problem. The receiver here is doing little more than walking the syntax tree which is done many, many times across Roslyn in the IDE. Can you elaborate more on what pattern this generator is using that is causing this behavior?

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality. And yes moving to the new APIs would "fix" the problem because it avoids the work. At the same time my suspicion is that is more masking the existing problem vs. fixing it.

sharwell · 2021-04-06T23:15:07Z

Can you elaborate more on what pattern this generator is using that is causing this behavior?

It's not walking a syntax tree, it's walking every syntax tree. In a large solution like this, the trees don't all fit in process address space at the same time, so a full walk across all the trees will cause many to get moved to temporary storage and others to be read back in. In a project small enough to hold all trees in the process address space without Gen 2 GC forcing them out, most of the allocations go away.

benaadams · 2021-04-06T23:17:04Z

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality.

Only does a name equality check if the class has an attribute with length 32 or length 23; which should be infrequent?

runtime/src/libraries/System.Private.CoreLib/generators/EventSourceGenerator.cs

Lines 69 to 83 in b3d4eb2

// Only clasess
if(syntaxNodeisClassDeclarationSyntaxclassDeclaration)
{
// Check if has EventSource attribute before adding to candidates
// as we don't want to add every class in the project
foreach(AttributeListSyntax?calinclassDeclaration.AttributeLists)
{
foreach(AttributeSyntax?caincal.Attributes)
{
// Check if Span length matches before allocating the string to check more
intlength=ca.Name.Span.Length;
if(length!=EventSourceAttribute.Length&&length!=EventSourceAttributeShort.Length)
{
continue;
}

jaredpar · 2021-04-06T23:23:26Z

Right so this is not a problem with the generator itself but more than the IDE hasn't yet move the generator infrastructure out of process. Hence you're still limited to the 32 bit address space in VS

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

sharwell · 2021-04-06T23:28:35Z

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

It's not pulling them all in at the same time. It's pulling them in as part of a background update, but it leads to significant churn. Items loaded early in one pass are released by the time the pass ends, which means they need to be reloaded during the next pass. When they are reloaded in the next pass, items at the end of the previous pass are released to make room, and those items then need to be reloaded.

The overhead could be reduced if ISyntaxReceiver had an equivalent that only needed the SyntaxTree, since the generator could avoid realizing the syntax tree if the text hash didn't change.

stephentoub · 2021-04-05T19:39:23Z

cc: @sharwell, @chsienki, @benaadams

I think we can get away with this for the EventSource generator, as the values it generates shouldn't be needed while working in the project. But I expect we won't be as lucky with the DllImportGenerator cc: @jkoritzinsky, @AaronRobinsonMSFT. @chsienki, is there a recommendation for how to avoid the impact here for such a generator? My understanding is you're working on a replacement set of APIs, but that we'll need to switch over to using them wholesale in order to get the benefits? Do you know when they'll be available?

ghost · 2021-04-05T19:39:29Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

sharwell

This is the approach that works for <Analyzer> references; someone with project system experience should verify that the same approach using <ProjectReference> would not be problematic.

stephentoub · 2021-04-06T02:35:51Z

I'm going to merge this. Typing in VS while working in Corelib.csproj is currently painful.

AaronRobinsonMSFT · 2021-04-06T02:55:27Z

@stephentoub This is unfortunate. I hope there are simply issues with the source generator impl rather than the overall design.

@elinor-fung and @jkoritzinsky I'm going to add an IDE integration validation step to #43060.

sharwell · 2021-04-06T05:07:47Z

@AaronRobinsonMSFT in the SG V1 API, it is very difficult to avoid observable typing lag (read: no known solution exists) when both of the following conditions are met, regardless of how fast the source generator itself is:

The project is large
The source generator uses a syntax receiver

I suggested @stephentoub be added to the working group for the SG V2 API, as I do not believe the P/Invoke source generator will be possible to sidestep either of the above, and the workaround in this PR is likely to not work.

stephentoub · 2021-04-06T13:16:18Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good. cc: @ericstj

@chsienki, when is the new API that addresses this going to be ready to consume? In time to rewrite in-box source generators to use it in .NET 6? Is there an issue tracking it?

…shim_mono # By Aaron Robinson (10) and others # Via GitHub * upstream/main: (108 commits) [mbr] Add Apple sample (dotnet#50740) make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763) Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622) [mono] More domain cleanups (dotnet#50479) Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754) Disable EventSource generator in design-time builds (dotnet#50741) Fix X509 test failures on Android (dotnet#50301) Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703) Enforce 64KB event payload size limit on EventPipe (dotnet#50600) Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906) [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458) improve connection scavenge logic by doing zero-byte read (dotnet#50545) Resolve call mdtokens when making tier 1 inline observations (dotnet#50675) Annotate APIs in System.Private.Xml (dotnet#49682) Support compiling against OpenSSL 3 headers Change Configuration.Json to use a regular Dictionary. (dotnet#50611) Remove unused BigNumFromBinary P/Invoke (dotnet#50670) Make Ninja the default CMake generator on Windows for the repo (dotnet#49715) [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637) [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547) ... # Conflicts: # src/mono/dlls/mscordbi/CMakeLists.txt

AaronRobinsonMSFT · 2021-04-06T16:27:44Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good.

Thankfully the Interop source generator will not be shipping publicly for .NET 6 - only for product build. However, we will want to ensure the product development loop isn't negative profoundly impacted. It sounds like a post-.NET 6 solution is likely so we will be able to consume that prior to an official beta for non-runtime consumption.

chsienki · 2021-04-06T17:41:56Z

@stephentoub We're targeting a preview for 16.10, but it'll require the user to opt-in until the APIs reach stable.

The issue tracking it is here dotnet/roslyn#51257 although it's not very well defined to be honest (I'll try and go through and create some more granular issues that link to that one for tracking).

Unsure if the timing is going to work for rewriting the inbox generators for the .NET 6 timeline, although we'd obviously like to do so.

jaredpar · 2021-04-06T21:59:31Z

Anyone have a good explanation of

The actual perf problem that was hit here?
Why they feel the new APIs are going to address that problem?

sharwell · 2021-04-06T22:06:47Z

The actual perf problem that was hit here?

The generator driver is allocating 14GB/min in GeneratorDriver.RunGenerators because we don't have any way to incrementally update the result for a source generator that uses a syntax receiver. The vast majority of these allocations are deserializing trees that moved to temporary storage since the whole solution doesn't fit in address space at the same time.

Why they feel the new APIs are going to address that problem?

The new pipeline API will allow an O(solution size) algorithm to reduce to an O(document size) algorithm for syntax receivers that have document granularity. In addition, the document needed for incremental update is much more likely to already be in memory because it's the document currently open in the editor.

jaredpar · 2021-04-06T23:08:33Z

There are plenty of syntax receiver based generators out there that do not exhibit this problem. The receiver here is doing little more than walking the syntax tree which is done many, many times across Roslyn in the IDE. Can you elaborate more on what pattern this generator is using that is causing this behavior?

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality. And yes moving to the new APIs would "fix" the problem because it avoids the work. At the same time my suspicion is that is more masking the existing problem vs. fixing it.

sharwell · 2021-04-06T23:15:07Z

Can you elaborate more on what pattern this generator is using that is causing this behavior?

It's not walking a syntax tree, it's walking every syntax tree. In a large solution like this, the trees don't all fit in process address space at the same time, so a full walk across all the trees will cause many to get moved to temporary storage and others to be read back in. In a project small enough to hold all trees in the process address space without Gen 2 GC forcing them out, most of the allocations go away.

benaadams · 2021-04-06T23:17:04Z

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality.

Only does a name equality check if the class has an attribute with length 32 or length 23; which should be infrequent?

runtime/src/libraries/System.Private.CoreLib/generators/EventSourceGenerator.cs

Lines 69 to 83 in b3d4eb2

// Only clasess
if(syntaxNodeisClassDeclarationSyntaxclassDeclaration)
{
// Check if has EventSource attribute before adding to candidates
// as we don't want to add every class in the project
foreach(AttributeListSyntax?calinclassDeclaration.AttributeLists)
{
foreach(AttributeSyntax?caincal.Attributes)
{
// Check if Span length matches before allocating the string to check more
intlength=ca.Name.Span.Length;
if(length!=EventSourceAttribute.Length&&length!=EventSourceAttributeShort.Length)
{
continue;
}

jaredpar · 2021-04-06T23:23:26Z

Right so this is not a problem with the generator itself but more than the IDE hasn't yet move the generator infrastructure out of process. Hence you're still limited to the 32 bit address space in VS

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

sharwell · 2021-04-06T23:28:35Z

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

It's not pulling them all in at the same time. It's pulling them in as part of a background update, but it leads to significant churn. Items loaded early in one pass are released by the time the pass ends, which means they need to be reloaded during the next pass. When they are reloaded in the next pass, items at the end of the previous pass are released to make room, and those items then need to be reloaded.

The overhead could be reduced if ISyntaxReceiver had an equivalent that only needed the SyntaxTree, since the generator could avoid realizing the syntax tree if the text hash didn't change.

stephentoub · 2021-04-05T19:39:23Z

cc: @sharwell, @chsienki, @benaadams

I think we can get away with this for the EventSource generator, as the values it generates shouldn't be needed while working in the project. But I expect we won't be as lucky with the DllImportGenerator cc: @jkoritzinsky, @AaronRobinsonMSFT. @chsienki, is there a recommendation for how to avoid the impact here for such a generator? My understanding is you're working on a replacement set of APIs, but that we'll need to switch over to using them wholesale in order to get the benefits? Do you know when they'll be available?

ghost · 2021-04-05T19:39:29Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

sharwell

This is the approach that works for <Analyzer> references; someone with project system experience should verify that the same approach using <ProjectReference> would not be problematic.

stephentoub · 2021-04-06T02:35:51Z

I'm going to merge this. Typing in VS while working in Corelib.csproj is currently painful.

AaronRobinsonMSFT · 2021-04-06T02:55:27Z

@stephentoub This is unfortunate. I hope there are simply issues with the source generator impl rather than the overall design.

@elinor-fung and @jkoritzinsky I'm going to add an IDE integration validation step to #43060.

sharwell · 2021-04-06T05:07:47Z

@AaronRobinsonMSFT in the SG V1 API, it is very difficult to avoid observable typing lag (read: no known solution exists) when both of the following conditions are met, regardless of how fast the source generator itself is:

The project is large
The source generator uses a syntax receiver

I suggested @stephentoub be added to the working group for the SG V2 API, as I do not believe the P/Invoke source generator will be possible to sidestep either of the above, and the workaround in this PR is likely to not work.

stephentoub · 2021-04-06T13:16:18Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good. cc: @ericstj

@chsienki, when is the new API that addresses this going to be ready to consume? In time to rewrite in-box source generators to use it in .NET 6? Is there an issue tracking it?

…shim_mono # By Aaron Robinson (10) and others # Via GitHub * upstream/main: (108 commits) [mbr] Add Apple sample (dotnet#50740) make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763) Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622) [mono] More domain cleanups (dotnet#50479) Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754) Disable EventSource generator in design-time builds (dotnet#50741) Fix X509 test failures on Android (dotnet#50301) Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703) Enforce 64KB event payload size limit on EventPipe (dotnet#50600) Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906) [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458) improve connection scavenge logic by doing zero-byte read (dotnet#50545) Resolve call mdtokens when making tier 1 inline observations (dotnet#50675) Annotate APIs in System.Private.Xml (dotnet#49682) Support compiling against OpenSSL 3 headers Change Configuration.Json to use a regular Dictionary. (dotnet#50611) Remove unused BigNumFromBinary P/Invoke (dotnet#50670) Make Ninja the default CMake generator on Windows for the repo (dotnet#49715) [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637) [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547) ... # Conflicts: # src/mono/dlls/mscordbi/CMakeLists.txt

AaronRobinsonMSFT · 2021-04-06T16:27:44Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good.

Thankfully the Interop source generator will not be shipping publicly for .NET 6 - only for product build. However, we will want to ensure the product development loop isn't negative profoundly impacted. It sounds like a post-.NET 6 solution is likely so we will be able to consume that prior to an official beta for non-runtime consumption.

chsienki · 2021-04-06T17:41:56Z

@stephentoub We're targeting a preview for 16.10, but it'll require the user to opt-in until the APIs reach stable.

The issue tracking it is here dotnet/roslyn#51257 although it's not very well defined to be honest (I'll try and go through and create some more granular issues that link to that one for tracking).

Unsure if the timing is going to work for rewriting the inbox generators for the .NET 6 timeline, although we'd obviously like to do so.

jaredpar · 2021-04-06T21:59:31Z

Anyone have a good explanation of

The actual perf problem that was hit here?
Why they feel the new APIs are going to address that problem?

sharwell · 2021-04-06T22:06:47Z

The actual perf problem that was hit here?

The generator driver is allocating 14GB/min in GeneratorDriver.RunGenerators because we don't have any way to incrementally update the result for a source generator that uses a syntax receiver. The vast majority of these allocations are deserializing trees that moved to temporary storage since the whole solution doesn't fit in address space at the same time.

Why they feel the new APIs are going to address that problem?

The new pipeline API will allow an O(solution size) algorithm to reduce to an O(document size) algorithm for syntax receivers that have document granularity. In addition, the document needed for incremental update is much more likely to already be in memory because it's the document currently open in the editor.

jaredpar · 2021-04-06T23:08:33Z

There are plenty of syntax receiver based generators out there that do not exhibit this problem. The receiver here is doing little more than walking the syntax tree which is done many, many times across Roslyn in the IDE. Can you elaborate more on what pattern this generator is using that is causing this behavior?

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality. And yes moving to the new APIs would "fix" the problem because it avoids the work. At the same time my suspicion is that is more masking the existing problem vs. fixing it.

sharwell · 2021-04-06T23:15:07Z

Can you elaborate more on what pattern this generator is using that is causing this behavior?

It's not walking a syntax tree, it's walking every syntax tree. In a large solution like this, the trees don't all fit in process address space at the same time, so a full walk across all the trees will cause many to get moved to temporary storage and others to be read back in. In a project small enough to hold all trees in the process address space without Gen 2 GC forcing them out, most of the allocations go away.

benaadams · 2021-04-06T23:17:04Z

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality.

Only does a name equality check if the class has an attribute with length 32 or length 23; which should be infrequent?

runtime/src/libraries/System.Private.CoreLib/generators/EventSourceGenerator.cs

Lines 69 to 83 in b3d4eb2

// Only clasess
if(syntaxNodeisClassDeclarationSyntaxclassDeclaration)
{
// Check if has EventSource attribute before adding to candidates
// as we don't want to add every class in the project
foreach(AttributeListSyntax?calinclassDeclaration.AttributeLists)
{
foreach(AttributeSyntax?caincal.Attributes)
{
// Check if Span length matches before allocating the string to check more
intlength=ca.Name.Span.Length;
if(length!=EventSourceAttribute.Length&&length!=EventSourceAttributeShort.Length)
{
continue;
}

jaredpar · 2021-04-06T23:23:26Z

Right so this is not a problem with the generator itself but more than the IDE hasn't yet move the generator infrastructure out of process. Hence you're still limited to the 32 bit address space in VS

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

sharwell · 2021-04-06T23:28:35Z

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

It's not pulling them all in at the same time. It's pulling them in as part of a background update, but it leads to significant churn. Items loaded early in one pass are released by the time the pass ends, which means they need to be reloaded during the next pass. When they are reloaded in the next pass, items at the end of the previous pass are released to make room, and those items then need to be reloaded.

The overhead could be reduced if ISyntaxReceiver had an equivalent that only needed the SyntaxTree, since the generator could avoid realizing the syntax tree if the text hash didn't change.

stephentoub · 2021-04-05T19:39:23Z

cc: @sharwell, @chsienki, @benaadams

I think we can get away with this for the EventSource generator, as the values it generates shouldn't be needed while working in the project. But I expect we won't be as lucky with the DllImportGenerator cc: @jkoritzinsky, @AaronRobinsonMSFT. @chsienki, is there a recommendation for how to avoid the impact here for such a generator? My understanding is you're working on a replacement set of APIs, but that we'll need to switch over to using them wholesale in order to get the benefits? Do you know when they'll be available?

ghost · 2021-04-05T19:39:29Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

sharwell

This is the approach that works for <Analyzer> references; someone with project system experience should verify that the same approach using <ProjectReference> would not be problematic.

stephentoub · 2021-04-06T02:35:51Z

I'm going to merge this. Typing in VS while working in Corelib.csproj is currently painful.

AaronRobinsonMSFT · 2021-04-06T02:55:27Z

@stephentoub This is unfortunate. I hope there are simply issues with the source generator impl rather than the overall design.

@elinor-fung and @jkoritzinsky I'm going to add an IDE integration validation step to #43060.

sharwell · 2021-04-06T05:07:47Z

@AaronRobinsonMSFT in the SG V1 API, it is very difficult to avoid observable typing lag (read: no known solution exists) when both of the following conditions are met, regardless of how fast the source generator itself is:

The project is large
The source generator uses a syntax receiver

I suggested @stephentoub be added to the working group for the SG V2 API, as I do not believe the P/Invoke source generator will be possible to sidestep either of the above, and the workaround in this PR is likely to not work.

stephentoub · 2021-04-06T13:16:18Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good. cc: @ericstj

@chsienki, when is the new API that addresses this going to be ready to consume? In time to rewrite in-box source generators to use it in .NET 6? Is there an issue tracking it?

…shim_mono # By Aaron Robinson (10) and others # Via GitHub * upstream/main: (108 commits) [mbr] Add Apple sample (dotnet#50740) make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763) Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622) [mono] More domain cleanups (dotnet#50479) Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754) Disable EventSource generator in design-time builds (dotnet#50741) Fix X509 test failures on Android (dotnet#50301) Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703) Enforce 64KB event payload size limit on EventPipe (dotnet#50600) Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906) [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458) improve connection scavenge logic by doing zero-byte read (dotnet#50545) Resolve call mdtokens when making tier 1 inline observations (dotnet#50675) Annotate APIs in System.Private.Xml (dotnet#49682) Support compiling against OpenSSL 3 headers Change Configuration.Json to use a regular Dictionary. (dotnet#50611) Remove unused BigNumFromBinary P/Invoke (dotnet#50670) Make Ninja the default CMake generator on Windows for the repo (dotnet#49715) [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637) [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547) ... # Conflicts: # src/mono/dlls/mscordbi/CMakeLists.txt

AaronRobinsonMSFT · 2021-04-06T16:27:44Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good.

Thankfully the Interop source generator will not be shipping publicly for .NET 6 - only for product build. However, we will want to ensure the product development loop isn't negative profoundly impacted. It sounds like a post-.NET 6 solution is likely so we will be able to consume that prior to an official beta for non-runtime consumption.

chsienki · 2021-04-06T17:41:56Z

@stephentoub We're targeting a preview for 16.10, but it'll require the user to opt-in until the APIs reach stable.

The issue tracking it is here dotnet/roslyn#51257 although it's not very well defined to be honest (I'll try and go through and create some more granular issues that link to that one for tracking).

Unsure if the timing is going to work for rewriting the inbox generators for the .NET 6 timeline, although we'd obviously like to do so.

jaredpar · 2021-04-06T21:59:31Z

Anyone have a good explanation of

The actual perf problem that was hit here?
Why they feel the new APIs are going to address that problem?

sharwell · 2021-04-06T22:06:47Z

The actual perf problem that was hit here?

The generator driver is allocating 14GB/min in GeneratorDriver.RunGenerators because we don't have any way to incrementally update the result for a source generator that uses a syntax receiver. The vast majority of these allocations are deserializing trees that moved to temporary storage since the whole solution doesn't fit in address space at the same time.

Why they feel the new APIs are going to address that problem?

The new pipeline API will allow an O(solution size) algorithm to reduce to an O(document size) algorithm for syntax receivers that have document granularity. In addition, the document needed for incremental update is much more likely to already be in memory because it's the document currently open in the editor.

jaredpar · 2021-04-06T23:08:33Z

There are plenty of syntax receiver based generators out there that do not exhibit this problem. The receiver here is doing little more than walking the syntax tree which is done many, many times across Roslyn in the IDE. Can you elaborate more on what pattern this generator is using that is causing this behavior?

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality. And yes moving to the new APIs would "fix" the problem because it avoids the work. At the same time my suspicion is that is more masking the existing problem vs. fixing it.

sharwell · 2021-04-06T23:15:07Z

Can you elaborate more on what pattern this generator is using that is causing this behavior?

It's not walking a syntax tree, it's walking every syntax tree. In a large solution like this, the trees don't all fit in process address space at the same time, so a full walk across all the trees will cause many to get moved to temporary storage and others to be read back in. In a project small enough to hold all trees in the process address space without Gen 2 GC forcing them out, most of the allocations go away.

benaadams · 2021-04-06T23:17:04Z

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality.

Only does a name equality check if the class has an attribute with length 32 or length 23; which should be infrequent?

runtime/src/libraries/System.Private.CoreLib/generators/EventSourceGenerator.cs

Lines 69 to 83 in b3d4eb2

// Only clasess
if(syntaxNodeisClassDeclarationSyntaxclassDeclaration)
{
// Check if has EventSource attribute before adding to candidates
// as we don't want to add every class in the project
foreach(AttributeListSyntax?calinclassDeclaration.AttributeLists)
{
foreach(AttributeSyntax?caincal.Attributes)
{
// Check if Span length matches before allocating the string to check more
intlength=ca.Name.Span.Length;
if(length!=EventSourceAttribute.Length&&length!=EventSourceAttributeShort.Length)
{
continue;
}

jaredpar · 2021-04-06T23:23:26Z

Right so this is not a problem with the generator itself but more than the IDE hasn't yet move the generator infrastructure out of process. Hence you're still limited to the 32 bit address space in VS

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

sharwell · 2021-04-06T23:28:35Z

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

It's not pulling them all in at the same time. It's pulling them in as part of a background update, but it leads to significant churn. Items loaded early in one pass are released by the time the pass ends, which means they need to be reloaded during the next pass. When they are reloaded in the next pass, items at the end of the previous pass are released to make room, and those items then need to be reloaded.

The overhead could be reduced if ISyntaxReceiver had an equivalent that only needed the SyntaxTree, since the generator could avoid realizing the syntax tree if the text hash didn't change.

stephentoub · 2021-04-05T19:39:23Z

cc: @sharwell, @chsienki, @benaadams

I think we can get away with this for the EventSource generator, as the values it generates shouldn't be needed while working in the project. But I expect we won't be as lucky with the DllImportGenerator cc: @jkoritzinsky, @AaronRobinsonMSFT. @chsienki, is there a recommendation for how to avoid the impact here for such a generator? My understanding is you're working on a replacement set of APIs, but that we'll need to switch over to using them wholesale in order to get the benefits? Do you know when they'll be available?

ghost · 2021-04-05T19:39:29Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

sharwell

This is the approach that works for <Analyzer> references; someone with project system experience should verify that the same approach using <ProjectReference> would not be problematic.

stephentoub · 2021-04-06T02:35:51Z

I'm going to merge this. Typing in VS while working in Corelib.csproj is currently painful.

AaronRobinsonMSFT · 2021-04-06T02:55:27Z

@stephentoub This is unfortunate. I hope there are simply issues with the source generator impl rather than the overall design.

@elinor-fung and @jkoritzinsky I'm going to add an IDE integration validation step to #43060.

sharwell · 2021-04-06T05:07:47Z

@AaronRobinsonMSFT in the SG V1 API, it is very difficult to avoid observable typing lag (read: no known solution exists) when both of the following conditions are met, regardless of how fast the source generator itself is:

The project is large
The source generator uses a syntax receiver

I suggested @stephentoub be added to the working group for the SG V2 API, as I do not believe the P/Invoke source generator will be possible to sidestep either of the above, and the workaround in this PR is likely to not work.

stephentoub · 2021-04-06T13:16:18Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good. cc: @ericstj

@chsienki, when is the new API that addresses this going to be ready to consume? In time to rewrite in-box source generators to use it in .NET 6? Is there an issue tracking it?

…shim_mono # By Aaron Robinson (10) and others # Via GitHub * upstream/main: (108 commits) [mbr] Add Apple sample (dotnet#50740) make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763) Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622) [mono] More domain cleanups (dotnet#50479) Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754) Disable EventSource generator in design-time builds (dotnet#50741) Fix X509 test failures on Android (dotnet#50301) Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703) Enforce 64KB event payload size limit on EventPipe (dotnet#50600) Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906) [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458) improve connection scavenge logic by doing zero-byte read (dotnet#50545) Resolve call mdtokens when making tier 1 inline observations (dotnet#50675) Annotate APIs in System.Private.Xml (dotnet#49682) Support compiling against OpenSSL 3 headers Change Configuration.Json to use a regular Dictionary. (dotnet#50611) Remove unused BigNumFromBinary P/Invoke (dotnet#50670) Make Ninja the default CMake generator on Windows for the repo (dotnet#49715) [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637) [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547) ... # Conflicts: # src/mono/dlls/mscordbi/CMakeLists.txt

AaronRobinsonMSFT · 2021-04-06T16:27:44Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good.

Thankfully the Interop source generator will not be shipping publicly for .NET 6 - only for product build. However, we will want to ensure the product development loop isn't negative profoundly impacted. It sounds like a post-.NET 6 solution is likely so we will be able to consume that prior to an official beta for non-runtime consumption.

chsienki · 2021-04-06T17:41:56Z

@stephentoub We're targeting a preview for 16.10, but it'll require the user to opt-in until the APIs reach stable.

The issue tracking it is here dotnet/roslyn#51257 although it's not very well defined to be honest (I'll try and go through and create some more granular issues that link to that one for tracking).

Unsure if the timing is going to work for rewriting the inbox generators for the .NET 6 timeline, although we'd obviously like to do so.

jaredpar · 2021-04-06T21:59:31Z

Anyone have a good explanation of

The actual perf problem that was hit here?
Why they feel the new APIs are going to address that problem?

sharwell · 2021-04-06T22:06:47Z

The actual perf problem that was hit here?

The generator driver is allocating 14GB/min in GeneratorDriver.RunGenerators because we don't have any way to incrementally update the result for a source generator that uses a syntax receiver. The vast majority of these allocations are deserializing trees that moved to temporary storage since the whole solution doesn't fit in address space at the same time.

Why they feel the new APIs are going to address that problem?

The new pipeline API will allow an O(solution size) algorithm to reduce to an O(document size) algorithm for syntax receivers that have document granularity. In addition, the document needed for incremental update is much more likely to already be in memory because it's the document currently open in the editor.

jaredpar · 2021-04-06T23:08:33Z

There are plenty of syntax receiver based generators out there that do not exhibit this problem. The receiver here is doing little more than walking the syntax tree which is done many, many times across Roslyn in the IDE. Can you elaborate more on what pattern this generator is using that is causing this behavior?

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality. And yes moving to the new APIs would "fix" the problem because it avoids the work. At the same time my suspicion is that is more masking the existing problem vs. fixing it.

sharwell · 2021-04-06T23:15:07Z

Can you elaborate more on what pattern this generator is using that is causing this behavior?

It's not walking a syntax tree, it's walking every syntax tree. In a large solution like this, the trees don't all fit in process address space at the same time, so a full walk across all the trees will cause many to get moved to temporary storage and others to be read back in. In a project small enough to hold all trees in the process address space without Gen 2 GC forcing them out, most of the allocations go away.

benaadams · 2021-04-06T23:17:04Z

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality.

Only does a name equality check if the class has an attribute with length 32 or length 23; which should be infrequent?

runtime/src/libraries/System.Private.CoreLib/generators/EventSourceGenerator.cs

Lines 69 to 83 in b3d4eb2

// Only clasess
if(syntaxNodeisClassDeclarationSyntaxclassDeclaration)
{
// Check if has EventSource attribute before adding to candidates
// as we don't want to add every class in the project
foreach(AttributeListSyntax?calinclassDeclaration.AttributeLists)
{
foreach(AttributeSyntax?caincal.Attributes)
{
// Check if Span length matches before allocating the string to check more
intlength=ca.Name.Span.Length;
if(length!=EventSourceAttribute.Length&&length!=EventSourceAttributeShort.Length)
{
continue;
}

jaredpar · 2021-04-06T23:23:26Z

Right so this is not a problem with the generator itself but more than the IDE hasn't yet move the generator infrastructure out of process. Hence you're still limited to the 32 bit address space in VS

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

sharwell · 2021-04-06T23:28:35Z

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

It's not pulling them all in at the same time. It's pulling them in as part of a background update, but it leads to significant churn. Items loaded early in one pass are released by the time the pass ends, which means they need to be reloaded during the next pass. When they are reloaded in the next pass, items at the end of the previous pass are released to make room, and those items then need to be reloaded.

The overhead could be reduced if ISyntaxReceiver had an equivalent that only needed the SyntaxTree, since the generator could avoid realizing the syntax tree if the text hash didn't change.

stephentoub · 2021-04-05T19:39:23Z

cc: @sharwell, @chsienki, @benaadams

I think we can get away with this for the EventSource generator, as the values it generates shouldn't be needed while working in the project. But I expect we won't be as lucky with the DllImportGenerator cc: @jkoritzinsky, @AaronRobinsonMSFT. @chsienki, is there a recommendation for how to avoid the impact here for such a generator? My understanding is you're working on a replacement set of APIs, but that we'll need to switch over to using them wholesale in order to get the benefits? Do you know when they'll be available?

ghost · 2021-04-05T19:39:29Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

sharwell

This is the approach that works for <Analyzer> references; someone with project system experience should verify that the same approach using <ProjectReference> would not be problematic.

stephentoub · 2021-04-06T02:35:51Z

I'm going to merge this. Typing in VS while working in Corelib.csproj is currently painful.

AaronRobinsonMSFT · 2021-04-06T02:55:27Z

@stephentoub This is unfortunate. I hope there are simply issues with the source generator impl rather than the overall design.

@elinor-fung and @jkoritzinsky I'm going to add an IDE integration validation step to #43060.

sharwell · 2021-04-06T05:07:47Z

@AaronRobinsonMSFT in the SG V1 API, it is very difficult to avoid observable typing lag (read: no known solution exists) when both of the following conditions are met, regardless of how fast the source generator itself is:

The project is large
The source generator uses a syntax receiver

I suggested @stephentoub be added to the working group for the SG V2 API, as I do not believe the P/Invoke source generator will be possible to sidestep either of the above, and the workaround in this PR is likely to not work.

stephentoub · 2021-04-06T13:16:18Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good. cc: @ericstj

@chsienki, when is the new API that addresses this going to be ready to consume? In time to rewrite in-box source generators to use it in .NET 6? Is there an issue tracking it?

…shim_mono # By Aaron Robinson (10) and others # Via GitHub * upstream/main: (108 commits) [mbr] Add Apple sample (dotnet#50740) make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763) Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622) [mono] More domain cleanups (dotnet#50479) Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754) Disable EventSource generator in design-time builds (dotnet#50741) Fix X509 test failures on Android (dotnet#50301) Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703) Enforce 64KB event payload size limit on EventPipe (dotnet#50600) Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906) [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458) improve connection scavenge logic by doing zero-byte read (dotnet#50545) Resolve call mdtokens when making tier 1 inline observations (dotnet#50675) Annotate APIs in System.Private.Xml (dotnet#49682) Support compiling against OpenSSL 3 headers Change Configuration.Json to use a regular Dictionary. (dotnet#50611) Remove unused BigNumFromBinary P/Invoke (dotnet#50670) Make Ninja the default CMake generator on Windows for the repo (dotnet#49715) [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637) [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547) ... # Conflicts: # src/mono/dlls/mscordbi/CMakeLists.txt

AaronRobinsonMSFT · 2021-04-06T16:27:44Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good.

Thankfully the Interop source generator will not be shipping publicly for .NET 6 - only for product build. However, we will want to ensure the product development loop isn't negative profoundly impacted. It sounds like a post-.NET 6 solution is likely so we will be able to consume that prior to an official beta for non-runtime consumption.

chsienki · 2021-04-06T17:41:56Z

@stephentoub We're targeting a preview for 16.10, but it'll require the user to opt-in until the APIs reach stable.

The issue tracking it is here dotnet/roslyn#51257 although it's not very well defined to be honest (I'll try and go through and create some more granular issues that link to that one for tracking).

Unsure if the timing is going to work for rewriting the inbox generators for the .NET 6 timeline, although we'd obviously like to do so.

jaredpar · 2021-04-06T21:59:31Z

Anyone have a good explanation of

The actual perf problem that was hit here?
Why they feel the new APIs are going to address that problem?

sharwell · 2021-04-06T22:06:47Z

The actual perf problem that was hit here?

The generator driver is allocating 14GB/min in GeneratorDriver.RunGenerators because we don't have any way to incrementally update the result for a source generator that uses a syntax receiver. The vast majority of these allocations are deserializing trees that moved to temporary storage since the whole solution doesn't fit in address space at the same time.

Why they feel the new APIs are going to address that problem?

The new pipeline API will allow an O(solution size) algorithm to reduce to an O(document size) algorithm for syntax receivers that have document granularity. In addition, the document needed for incremental update is much more likely to already be in memory because it's the document currently open in the editor.

jaredpar · 2021-04-06T23:08:33Z

There are plenty of syntax receiver based generators out there that do not exhibit this problem. The receiver here is doing little more than walking the syntax tree which is done many, many times across Roslyn in the IDE. Can you elaborate more on what pattern this generator is using that is causing this behavior?

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality. And yes moving to the new APIs would "fix" the problem because it avoids the work. At the same time my suspicion is that is more masking the existing problem vs. fixing it.

sharwell · 2021-04-06T23:15:07Z

Can you elaborate more on what pattern this generator is using that is causing this behavior?

It's not walking a syntax tree, it's walking every syntax tree. In a large solution like this, the trees don't all fit in process address space at the same time, so a full walk across all the trees will cause many to get moved to temporary storage and others to be read back in. In a project small enough to hold all trees in the process address space without Gen 2 GC forcing them out, most of the allocations go away.

benaadams · 2021-04-06T23:17:04Z

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality.

Only does a name equality check if the class has an attribute with length 32 or length 23; which should be infrequent?

runtime/src/libraries/System.Private.CoreLib/generators/EventSourceGenerator.cs

Lines 69 to 83 in b3d4eb2

// Only clasess
if(syntaxNodeisClassDeclarationSyntaxclassDeclaration)
{
// Check if has EventSource attribute before adding to candidates
// as we don't want to add every class in the project
foreach(AttributeListSyntax?calinclassDeclaration.AttributeLists)
{
foreach(AttributeSyntax?caincal.Attributes)
{
// Check if Span length matches before allocating the string to check more
intlength=ca.Name.Span.Length;
if(length!=EventSourceAttribute.Length&&length!=EventSourceAttributeShort.Length)
{
continue;
}

jaredpar · 2021-04-06T23:23:26Z

Right so this is not a problem with the generator itself but more than the IDE hasn't yet move the generator infrastructure out of process. Hence you're still limited to the 32 bit address space in VS

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

sharwell · 2021-04-06T23:28:35Z

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

It's not pulling them all in at the same time. It's pulling them in as part of a background update, but it leads to significant churn. Items loaded early in one pass are released by the time the pass ends, which means they need to be reloaded during the next pass. When they are reloaded in the next pass, items at the end of the previous pass are released to make room, and those items then need to be reloaded.

The overhead could be reduced if ISyntaxReceiver had an equivalent that only needed the SyntaxTree, since the generator could avoid realizing the syntax tree if the text hash didn't change.

stephentoub · 2021-04-05T19:39:23Z

cc: @sharwell, @chsienki, @benaadams

I think we can get away with this for the EventSource generator, as the values it generates shouldn't be needed while working in the project. But I expect we won't be as lucky with the DllImportGenerator cc: @jkoritzinsky, @AaronRobinsonMSFT. @chsienki, is there a recommendation for how to avoid the impact here for such a generator? My understanding is you're working on a replacement set of APIs, but that we'll need to switch over to using them wholesale in order to get the benefits? Do you know when they'll be available?

ghost · 2021-04-05T19:39:29Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

sharwell

This is the approach that works for <Analyzer> references; someone with project system experience should verify that the same approach using <ProjectReference> would not be problematic.

stephentoub · 2021-04-06T02:35:51Z

I'm going to merge this. Typing in VS while working in Corelib.csproj is currently painful.

AaronRobinsonMSFT · 2021-04-06T02:55:27Z

@stephentoub This is unfortunate. I hope there are simply issues with the source generator impl rather than the overall design.

@elinor-fung and @jkoritzinsky I'm going to add an IDE integration validation step to #43060.

sharwell · 2021-04-06T05:07:47Z

@AaronRobinsonMSFT in the SG V1 API, it is very difficult to avoid observable typing lag (read: no known solution exists) when both of the following conditions are met, regardless of how fast the source generator itself is:

The project is large
The source generator uses a syntax receiver

I suggested @stephentoub be added to the working group for the SG V2 API, as I do not believe the P/Invoke source generator will be possible to sidestep either of the above, and the workaround in this PR is likely to not work.

stephentoub · 2021-04-06T13:16:18Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good. cc: @ericstj

@chsienki, when is the new API that addresses this going to be ready to consume? In time to rewrite in-box source generators to use it in .NET 6? Is there an issue tracking it?

…shim_mono # By Aaron Robinson (10) and others # Via GitHub * upstream/main: (108 commits) [mbr] Add Apple sample (dotnet#50740) make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763) Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622) [mono] More domain cleanups (dotnet#50479) Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754) Disable EventSource generator in design-time builds (dotnet#50741) Fix X509 test failures on Android (dotnet#50301) Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703) Enforce 64KB event payload size limit on EventPipe (dotnet#50600) Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906) [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458) improve connection scavenge logic by doing zero-byte read (dotnet#50545) Resolve call mdtokens when making tier 1 inline observations (dotnet#50675) Annotate APIs in System.Private.Xml (dotnet#49682) Support compiling against OpenSSL 3 headers Change Configuration.Json to use a regular Dictionary. (dotnet#50611) Remove unused BigNumFromBinary P/Invoke (dotnet#50670) Make Ninja the default CMake generator on Windows for the repo (dotnet#49715) [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637) [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547) ... # Conflicts: # src/mono/dlls/mscordbi/CMakeLists.txt

AaronRobinsonMSFT · 2021-04-06T16:27:44Z

This needs to be factored in for any of us considering shipping source generators in .NET 6. The experience right now is not good.

Thankfully the Interop source generator will not be shipping publicly for .NET 6 - only for product build. However, we will want to ensure the product development loop isn't negative profoundly impacted. It sounds like a post-.NET 6 solution is likely so we will be able to consume that prior to an official beta for non-runtime consumption.

chsienki · 2021-04-06T17:41:56Z

@stephentoub We're targeting a preview for 16.10, but it'll require the user to opt-in until the APIs reach stable.

The issue tracking it is here dotnet/roslyn#51257 although it's not very well defined to be honest (I'll try and go through and create some more granular issues that link to that one for tracking).

Unsure if the timing is going to work for rewriting the inbox generators for the .NET 6 timeline, although we'd obviously like to do so.

jaredpar · 2021-04-06T21:59:31Z

Anyone have a good explanation of

The actual perf problem that was hit here?
Why they feel the new APIs are going to address that problem?

sharwell · 2021-04-06T22:06:47Z

The actual perf problem that was hit here?

The generator driver is allocating 14GB/min in GeneratorDriver.RunGenerators because we don't have any way to incrementally update the result for a source generator that uses a syntax receiver. The vast majority of these allocations are deserializing trees that moved to temporary storage since the whole solution doesn't fit in address space at the same time.

Why they feel the new APIs are going to address that problem?

The new pipeline API will allow an O(solution size) algorithm to reduce to an O(document size) algorithm for syntax receivers that have document granularity. In addition, the document needed for incremental update is much more likely to already be in memory because it's the document currently open in the editor.

jaredpar · 2021-04-06T23:08:33Z

There are plenty of syntax receiver based generators out there that do not exhibit this problem. The receiver here is doing little more than walking the syntax tree which is done many, many times across Roslyn in the IDE. Can you elaborate more on what pattern this generator is using that is causing this behavior?

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality. And yes moving to the new APIs would "fix" the problem because it avoids the work. At the same time my suspicion is that is more masking the existing problem vs. fixing it.

sharwell · 2021-04-06T23:15:07Z

Can you elaborate more on what pattern this generator is using that is causing this behavior?

It's not walking a syntax tree, it's walking every syntax tree. In a large solution like this, the trees don't all fit in process address space at the same time, so a full walk across all the trees will cause many to get moved to temporary storage and others to be read back in. In a project small enough to hold all trees in the process address space without Gen 2 GC forcing them out, most of the allocations go away.

benaadams · 2021-04-06T23:17:04Z

Looking at the generator I see a few potential issues, definitely some allocations that could be avoided if they switched to type equality vs. name equality.

Only does a name equality check if the class has an attribute with length 32 or length 23; which should be infrequent?

runtime/src/libraries/System.Private.CoreLib/generators/EventSourceGenerator.cs

Lines 69 to 83 in b3d4eb2

// Only clasess
if(syntaxNodeisClassDeclarationSyntaxclassDeclaration)
{
// Check if has EventSource attribute before adding to candidates
// as we don't want to add every class in the project
foreach(AttributeListSyntax?calinclassDeclaration.AttributeLists)
{
foreach(AttributeSyntax?caincal.Attributes)
{
// Check if Span length matches before allocating the string to check more
intlength=ca.Name.Span.Length;
if(length!=EventSourceAttribute.Length&&length!=EventSourceAttributeShort.Length)
{
continue;
}

jaredpar · 2021-04-06T23:23:26Z

Right so this is not a problem with the generator itself but more than the IDE hasn't yet move the generator infrastructure out of process. Hence you're still limited to the 32 bit address space in VS

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

sharwell · 2021-04-06T23:28:35Z

Why is the IDE pulling them all into memory at the same time here? Yes the generator is caching a subset of the trees but not enough to hit the constraints you are mentioning. I would assume the trees are brought in sequentially here, or in parallel, but not clear why everything is being held in memory at once.

It's not pulling them all in at the same time. It's pulling them in as part of a background update, but it leads to significant churn. Items loaded early in one pass are released by the time the pass ends, which means they need to be reloaded during the next pass. When they are reloaded in the next pass, items at the end of the previous pass are released to make room, and those items then need to be reloaded.

The overhead could be reduced if ISyntaxReceiver had an equivalent that only needed the SyntaxTree, since the generator could avoid realizing the syntax tree if the text hash didn't change.

Disable EventSource generator in design-time builds
1d7ad6d

sharwell approved these changes Apr 5, 2021
View reviewed changes

stephentoub merged commit 29c911e into dotnet:mainApr 6, 2021

stephentoub deleted the esgdesign branch April 6, 2021 02:35

AaronRobinsonMSFT mentioned this pull request Apr 6, 2021
Source generation of ILStub for DllImport #43060
Closed
50 tasks

ghost locked as resolved and limited conversation to collaborators May 7, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Disable EventSource generator in design-time builds
1d7ad6d

sharwell approved these changes Apr 5, 2021
View reviewed changes

stephentoub merged commit 29c911e into dotnet:mainApr 6, 2021

stephentoub deleted the esgdesign branch April 6, 2021 02:35

AaronRobinsonMSFT mentioned this pull request Apr 6, 2021
Source generation of ILStub for DllImport #43060
Closed
50 tasks

ghost locked as resolved and limited conversation to collaborators May 7, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Disable EventSource generator in design-time builds
1d7ad6d

sharwell approved these changes Apr 5, 2021
View reviewed changes

stephentoub merged commit 29c911e into dotnet:mainApr 6, 2021

stephentoub deleted the esgdesign branch April 6, 2021 02:35

AaronRobinsonMSFT mentioned this pull request Apr 6, 2021
Source generation of ILStub for DllImport #43060
Closed
50 tasks

ghost locked as resolved and limited conversation to collaborators May 7, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Disable EventSource generator in design-time builds
1d7ad6d

sharwell approved these changes Apr 5, 2021
View reviewed changes

stephentoub merged commit 29c911e into dotnet:mainApr 6, 2021

stephentoub deleted the esgdesign branch April 6, 2021 02:35

AaronRobinsonMSFT mentioned this pull request Apr 6, 2021
Source generation of ILStub for DllImport #43060
Closed
50 tasks

ghost locked as resolved and limited conversation to collaborators May 7, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Disable EventSource generator in design-time builds
1d7ad6d

sharwell approved these changes Apr 5, 2021
View reviewed changes

stephentoub merged commit 29c911e into dotnet:mainApr 6, 2021

stephentoub deleted the esgdesign branch April 6, 2021 02:35

AaronRobinsonMSFT mentioned this pull request Apr 6, 2021
Source generation of ILStub for DllImport #43060
Closed
50 tasks

ghost locked as resolved and limited conversation to collaborators May 7, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Disable EventSource generator in design-time builds
1d7ad6d

sharwell approved these changes Apr 5, 2021
View reviewed changes

stephentoub merged commit 29c911e into dotnet:mainApr 6, 2021

stephentoub deleted the esgdesign branch April 6, 2021 02:35

AaronRobinsonMSFT mentioned this pull request Apr 6, 2021
Source generation of ILStub for DllImport #43060
Closed
50 tasks

ghost locked as resolved and limited conversation to collaborators May 7, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Disable EventSource generator in design-time builds
1d7ad6d

sharwell approved these changes Apr 5, 2021
View reviewed changes

stephentoub merged commit 29c911e into dotnet:mainApr 6, 2021

stephentoub deleted the esgdesign branch April 6, 2021 02:35

AaronRobinsonMSFT mentioned this pull request Apr 6, 2021
Source generation of ILStub for DllImport #43060
Closed
50 tasks

ghost locked as resolved and limited conversation to collaborators May 7, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Disable EventSource generator in design-time builds
1d7ad6d

sharwell approved these changes Apr 5, 2021
View reviewed changes

stephentoub merged commit 29c911e into dotnet:mainApr 6, 2021

stephentoub deleted the esgdesign branch April 6, 2021 02:35

AaronRobinsonMSFT mentioned this pull request Apr 6, 2021
Source generation of ILStub for DllImport #43060
Closed
50 tasks

ghost locked as resolved and limited conversation to collaborators May 7, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Uh oh!

Conversation

stephentoub commented Apr 5, 2021

Uh oh!

ghost commented Apr 5, 2021

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Apr 6, 2021

Uh oh!

AaronRobinsonMSFT commented Apr 6, 2021

Uh oh!

sharwell commented Apr 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Apr 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronRobinsonMSFT commented Apr 6, 2021

Uh oh!

chsienki commented Apr 6, 2021

Uh oh!

jaredpar commented Apr 6, 2021

Uh oh!

sharwell commented Apr 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar commented Apr 6, 2021

Uh oh!

sharwell commented Apr 6, 2021

Uh oh!

benaadams commented Apr 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar commented Apr 6, 2021

Uh oh!

sharwell commented Apr 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

stephentoub commented Apr 5, 2021

Uh oh!

ghost commented Apr 5, 2021

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Apr 6, 2021

Uh oh!

AaronRobinsonMSFT commented Apr 6, 2021

Uh oh!

sharwell commented Apr 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Apr 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronRobinsonMSFT commented Apr 6, 2021

Uh oh!

chsienki commented Apr 6, 2021

Uh oh!

jaredpar commented Apr 6, 2021

Uh oh!

sharwell commented Apr 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar commented Apr 6, 2021

Uh oh!

sharwell commented Apr 6, 2021

Uh oh!

benaadams commented Apr 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

sharwell commented Apr 6, 2021•
edited
Loading

stephentoub commented Apr 6, 2021•
edited
Loading

sharwell commented Apr 6, 2021•
edited
Loading

benaadams commented Apr 6, 2021•
edited
Loading

sharwell commented Apr 6, 2021•
edited
Loading

stephentoub commented Apr 6, 2021•
edited
Loading

sharwell commented Apr 6, 2021•
edited
Loading

benaadams commented Apr 6, 2021•
edited
Loading

sharwell commented Apr 6, 2021•
edited
Loading

stephentoub commented Apr 6, 2021•
edited
Loading

sharwell commented Apr 6, 2021•
edited
Loading

benaadams commented Apr 6, 2021•
edited
Loading