Avoid a capturing local variables on the GetOptionsAsync fast path #23554
Conversation
| @@ -475,17 +475,22 @@ internal Task<DocumentOptionSet> GetOptionsAsync(OptionSet solutionOptions, Canc | |||
| // snapshot model. once that is fixed, we can remove this workaround - https://github.com/dotnet/roslyn/issues/19284 | |||
| if (_cachedOptions == null) | |||
| { | |||
| var newAsyncLazy = new AsyncLazy<DocumentOptionSet>(async c => | |||
There was a problem hiding this comment.
How does this prevent capturing a local? It's seems to me that it's moved to an instance method with the exact same code, which I don't think has any effect on lambda captures.
There was a problem hiding this comment.
➡️ In the fast path, _cachedOptions is not null.
There was a problem hiding this comment.
Oh, so it completely avoids allocating the lambda capturing this and locals. I get it now.
There was a problem hiding this comment.
Shouldn't codegen be actually handling this case?
There was a problem hiding this comment.
I'm not sure. @dotnet/roslyn-compiler do you have more information and/or a link to an existing issue for this?
There was a problem hiding this comment.
@agocke for more insight here.
There was a problem hiding this comment.
Captured variable frames are created in the scope the variable is declared. This is an implementation detail of the compiler, so it is not documented and probably shouldn't be.
There is a feature request to use more advanced analysis to detect when the frame could be created late, but it's not something we've done before: #20777
| } | ||
|
|
||
| return _cachedOptions.GetValueAsync(cancellationToken); | ||
| } | ||
|
|
||
| private void InitializeCachedOptions(OptionSet solutionOptions, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Probably should add a comment that this is split into a separate function for the reason you're doing this.
There was a problem hiding this comment.
➡️ Documented via an attribute which can be enforced in the future
sharwell
force-pushed
the
avoid-capturing
branch
from
3ded987
to
fc628b5
Compare
|
@Pilchie for ask mode |
| /// </remarks> | ||
| [Conditional("EMIT_CODE_ANALYSIS_ATTRIBUTES")] | ||
| [AttributeUsage(AttributeTargets.Constructor | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = true, Inherited = false)] | ||
| internal sealed class PerformanceSensitiveAttribute : Attribute |
There was a problem hiding this comment.
This attribute should also be applied to this locations to avoid regressions:
PR #23490
SourceNamespaceSymbol
IsDefinedInSourceTree
Customer scenario
Running analyzer during a build is slower than it should be, with the analyzer driver contributing substantial overhead even when the analyzers themselves are lightweight.
Bugs this fixes
N/A
Workarounds, if any
None needed
Risk
Low. The performance constraint is documented, and I filed dotnet/roslyn-analyzers#1438 in hope of enforcing the constraint at build time in the future.
Performance impact
AnalyzerRunner indicates allocation savings of 1.5% (1.30GiB).
Is this a regression from a previous update?
No.
Root cause analysis
AnalyzerRunner is a new tool for helping us test analyzer performance in isolation.
How was the bug found?
AnalyzerRunner.
Test documentation updated?
No.