Cache semantic models per syntax tree#4203
Conversation
|
LGTM, though I'd like to get @chsienki approval. |
Codecov Report
@@ Coverage Diff @@
## master #4203 +/- ##
===================================================
- Coverage 98.12512% 98.11738% -0.00774%
===================================================
Files 497 497
Lines 258523 258523
Branches 4492 4492
===================================================
- Hits 253676 253656 -20
- Misses 4089 4105 +16
- Partials 758 762 +4
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Oh, the AB# thing doesn't work here? You should talk to @Pilchie about that 😁 |
| // The compiler doesn't necessarily cache semantic models for a single syntax tree | ||
| // so we will do that here, ensuring we only do the expensive work once per tree. | ||
| // We can't cache this at a higher level because generator lifetime is not to be relied on. | ||
| var semanticModelCache = new Dictionary<SyntaxTree, SemanticModel>(); |
There was a problem hiding this comment.
This is interesting. We'll definitely fix the semantic model caching, but its a good point that there's no way to cache things in a more general sense. An interesting balance because we don't want you to cache the compilation, syntax trees, etc. (or the semantic model) but there might be things the generator could cache.
Nothing to do with this PR, but an interesting observation 👍
There was a problem hiding this comment.
Yeah, I mainly wrote the comment because I figured otherwise the first question on the PR would be "why not make this a field?".
Sadly, blindly calling GetSemanticModel, and trying to avoid state, are both super easy traps to fall in to. Luckily I knew about one of them :)
|
Thank you |
|
Do we have a bug tracknig the fix on the Rsolyn side? |
|
Issue is here: dotnet/roslyn#49289 |
Hopefully this is a mitigation for AB#1241130 before the real work is done in the compiler.
Microsoft Reviewers: Open in CodeFlow