Conversation
jasonmalinowski
left a comment
There was a problem hiding this comment.
Code is good but please coordinate with the myget removal effort prior to merging this.
Can you post the results of the before / after for the analyzer perf tests you run on Roslyn? The stress tool you have. Would be nice to see the real gains here. |
|
@jaredpar The changes seemed to save ~20 seconds for a command line build with analyzers. I can't get meaningful relative number prior to the named pipes fix in VBCSCompiler (currently the build doesn't use the compiler server, which causes more than a 2:1 increase in build times). |
Youssef1313
left a comment
There was a problem hiding this comment.
@sharwell I think there is no longer need for the myget reference. So this PR is no longer blocked and can be worked on.
8cdc5ca to
ecc3027
Compare
Curious: why does this result in a perf win? My intuition is that it would be cheaper to do syntax based checks when possible. |
Syntax checks are faster when checking syntax. In this case, the syntax checks were using SemanticModel to access symbol information. Rather than use the SemanticModel on a syntax path that's expected to be fast, we use a symbol callback that occurs later and provides the necessary symbol information up front / without extra cost. |
|
@sharwell What do you think we should do with this PR? |
|
Now updated again, should merge when the build finishes. |
The new analyzers are primarily IOperation based, which substantially improves build performance. Most of the performance benefits come from the following changes: