-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Throttle designer attribute processing. #67056
Throttle designer attribute processing. #67056
Conversation
_documentToLastReportedInformation.TryGetValue(data.DocumentId, out var existingInfo); | ||
if (existingInfo.category != data.Category) | ||
results.Add(data); | ||
if (data?.Category != existingInfo.category) |
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.
if (data?.Category != existingInfo.category) | |
if (data.Value.Category != existingInfo.category) |
var designerCategoryType = compilation.DesignerCategoryAttributeType(); | ||
if (designerCategoryType == null) | ||
return; | ||
// Defer expensive work until it's actually needed. |
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.
made values lazy to defer until needed.
{ | ||
var compilation = await project.GetRequiredCompilationAsync(cancellationToken).ConfigureAwait(false); | ||
return compilation.DesignerCategoryAttributeType() != null; | ||
}, cacheResult: true)); |
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.
figuring out if a set of references contains the DesignCategoryAttribute can be computed/cached for that set of references. As those rarely change for a project, it's much more efficient than having to continually use the project and create the compilation for it.
} | ||
|
||
using var _2 = ArrayBuilder<DesignerAttributeData>.GetInstance(tasks.Count, out var results); |
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.
removed parallel processing here. this runs in the bg, and uses resources that active features (like lightbulbs) want to be using.
} | ||
|
||
public static async Task<string?> ComputeDesignerAttributeCategoryAsync( | ||
AsyncLazy<bool> lazyHasDesignerCategoryType, Document document, CancellationToken cancellationToken) |
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.
helper was in a sibling file. just inlined into this.
if (designerCategoryType == null) | ||
return; | ||
// Defer expensive work until it's actually needed. | ||
var lazyProjectVersion = AsyncLazy.Create(project.GetSemanticVersionAsync, cacheResult: true); |
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.
The original code invoked GetDependentSemanticVersionAsync
. Is the change to GetSemanticVersionAsync
intentional here?
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. will add comment.
CancellationToken cancellationToken) | ||
{ | ||
using var _1 = ArrayBuilder<Task<DesignerAttributeData?>>.GetInstance(out var tasks); | ||
using var _ = ArrayBuilder<(DesignerAttributeData data, VersionStamp version)>.GetInstance(out var results); | ||
foreach (var document in project.Documents) |
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.
Probably add a comment here why we prefer to process documents sequentially instead of concurrently?
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.
sure. will do.
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. I'll validate the performance numbers on top of this PR. Thanks!
This service runs in the BG, so we don't want it saturating OOP with too much work. THere are a few major changes here.