-
Notifications
You must be signed in to change notification settings - Fork 231
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
Cache CreateCfg #5403
Cache CreateCfg #5403
Conversation
e7f17d2
to
13988af
Compare
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
|
||
public ControlFlowGraph FindOrCreate(SyntaxNode declaration, SemanticModel model) | ||
{ | ||
var rootSyntax = model.GetOperation(declaration).RootOperation().Syntax; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand how this is meant to work. If we take the example from CreateCfg_Performance_UsesCache_CS
and we request the CFG for the lambda, we will receive the CFG of the root operation this is the method declaration. Is this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. Roslyn doesn't let us directly create CFG for Lambda/Local function. We need to create a CFG from the root (method). And find our desired CFG in the CFG tree.
That's also the reason for the high cache hit rate.
SyntaxNodeExtensionsCS.CreateCfg(lambda, model); | ||
} | ||
}; | ||
a.ExecutionTime().Should().BeLessThan(1.Seconds()); // Takes roughly 0.2 sec on CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried to run without the cache? I'm curious about the improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roughly 5x slower on my machine without cache on this very small method. The bigger the method is, the worse it gets.
I used the full user repro project build to see how it works. It went down from 8 minutes to 20 sec with the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #5384
When analyzing user project with only S1854 active, I get 20 sec build&analysis time after this change.
For 16000 invocations on that project, we have 11086 cache hits and 4914 cache misses.