-
Notifications
You must be signed in to change notification settings - Fork 196
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
Improve perf in generator cache cases #10577
Changes from 1 commit
3606e2d
eb903b7
bfbd9cc
968afab
88aadb0
23b9433
7db79f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,7 +145,20 @@ public void Initialize(IncrementalGeneratorInitializationContext context) | |
var ((compilationA, razorSourceGeneratorOptionsA), hasRazorFilesA) = a; | ||
var ((compilationB, razorSourceGeneratorOptionsB), hasRazorFilesB) = b; | ||
|
||
if (!compilationA.References.SequenceEqual(compilationB.References)) | ||
// when using the generator cache in the compiler its possible to encounter metadata references that are different instances | ||
chsienki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// but ultimately represent the same underlying assembly. We compare the module version ids to determine if the references are the same | ||
if (!compilationA.References.SequenceEqual(compilationB.References, new LambdaComparer<MetadataReference>((old, @new) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Should we cache this LambdaComparer in some static field to avoid allocating it on each call? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't because the internal lambda is capturing the outer compilations. |
||
{ | ||
if (old is null || @new is null) | ||
{ | ||
return false; | ||
} | ||
|
||
return compilationA.GetAssemblyOrModuleSymbol(old) is IAssemblySymbol oldAssembly | ||
&& compilationB.GetAssemblyOrModuleSymbol(@new) is IAssemblySymbol newAssembly | ||
&& oldAssembly.Modules.Select(m => m.GetMetadata()?.GetModuleVersionId() ?? Guid.Empty) | ||
.SequenceEqual(newAssembly.Modules.Select(m => m.GetMetadata()?.GetModuleVersionId() ?? Guid.Empty)); | ||
}))) | ||
{ | ||
return false; | ||
} | ||
|
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.
This was never actually used, but incurred cost enumerating the references.
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.
Doesn't seem to be hooked up on the tooling side. Perhaps we can delete the feature?
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.
Yeah, looks like its only tests using it now. Will submit a follow up to clean this up.