Add a cache for AnalyzerConfigSets #46740
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mavasani
approved these changes
Aug 12, 2020
src/Workspaces/Core/Portable/Workspace/Solution/CachingAnalyzerConfigSet.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public AnalyzerConfigOptionsResult GetOptionsForSourcePath(string sourcePath) | ||
| { | ||
| return _cache.GetOrAdd(sourcePath, _computeFunction); |
There was a problem hiding this comment.
hrmm.. why not have the cachign in teh ANalyzerConfigSet itself?
There was a problem hiding this comment.
The compiler also calls into AnalyerConfigSet and does it's own (different) caching atop it, so I didn't want to push that right away, at least not as something we'll be taking as a hotfix.
There was a problem hiding this comment.
And more generally: I'm not sure if we'll want smarter caching policies here to limit memory usage, so I think we're best off keeping caching managed by the caller.
The change to move .editorconfig off of syntax trees meant that we were recomputing a bunch of information again and again, since previously the cached syntax trees implicitly acted as a cache of this data. This adds a quick cache back to avoid some extra overhead. This cache is a bit more expensive than it probably needs to be, but it's not too bad as the underlying AnalyerConfigSet has some logic to avoid recreating duplicate arrays/dictionaries of the underlying data, so much of the data is ultimately shared in the end.
jasonmalinowski
force-pushed
the
add-editorconfig-cache
branch
from
ec772f5 to
7480979
Compare
|
Hello @jasonmalinowski! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The change to move .editorconfig off of syntax trees meant that we were recomputing a bunch of information again and again, since previously the cached syntax trees implicitly acted as a cache of this data. This adds a quick cache back to avoid some extra overhead.
Ask Mode
Who is impacted by this bug?
Anybody who tries using a solution with an .editorconfig that is big enough to have scale issues. (Roslyn is notably impacted.)
Bugs Fixed
We don't have a tracking bug -- we just wrote the PR as soon as we were aware of the issue.
What is the customer scenario and impact of the bug?
If you have solutions with .editorconfigs, a bunch of features will slow down. For example, making a change in Roslyn and waiting for squiggles to show for that change is slowed down from a few seconds to almost a minute.
What is the workaround?
None.
How was the bug found?
Dogfooding.
If this fix is for a regression - what had regressed, when was the regression introduced, and why was the regression originally missed?
Insufficient testing of #44331. The original change was a performance fix for one scenario, but this regressed the perf for a different change. RPS doesn't have any scenarios where we test a solution with an .editorconfig, so the path that regressed is never tested. We are working on making such a test solution long term.
Testing
Manually verified by two different developers. Long term, an automated test is being worked on to catch this.