-
Notifications
You must be signed in to change notification settings - Fork 191
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
Reduce update frequency from RazorWorkspaceListener #10756
Conversation
...src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Serialization/RazorMessagePackSerializer.cs
Show resolved
Hide resolved
…ialization/RazorMessagePackSerializer.cs
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 took a pass through and added some comments. Beyond the minor feedback, I strongly suggest not relying on GetHashCode()
. Instead, use our Checksum
infrastructure to produce stronger identity for the cache.
.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/RazorProjectInfoHelpers.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/RazorProjectInfoHelpers.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/RazorProjectInfoHelpers.cs
Outdated
Show resolved
Hide resolved
...src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Serialization/RazorMessagePackSerializer.cs
Show resolved
Hide resolved
...azor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/CachedTagHelperResolver.DeltaResult.cs
Outdated
Show resolved
Hide resolved
...azor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/CachedTagHelperResolver.DeltaResult.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/CachedTagHelperResolver.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/CachedTagHelperResolver.cs
Show resolved
Hide resolved
return (false, null); | ||
} | ||
|
||
if (entry.DocumentsHash != documents.GetHashCode()) |
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 won't do what you want. Instead, it'll likely return true almost every time. RazorProjectInfoHelpers.GetDocumts()
returns a new ImmutableArray<DocumentSnapshotHandle>
each time it's called, and ImmutableArray<T>.GetHashCode()
isn't computed from the contents; it just passes the reference to the underlying array.
To address this, please use Microsoft.AspNetCore.Razor.Utilities.Checksum
and build up a checksum.
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 didn't look too closely at anything after my last comment, because it seemed like Dustin covered it. Definitely +1 to the GetHashCode()
usage, it definitely reads as potentially incorrect (eg, even though RazorConfiguration
defines an override, is it handling its Extensions
property correctly??). If it isn't, then comments confirming that would be appreciated.
.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs
Show resolved
Hide resolved
.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs
Outdated
Show resolved
Hide resolved
var csharpLanguageVersion = (project.ParseOptions as CSharpParseOptions)?.LanguageVersion ?? LanguageVersion.Default; | ||
CachedTagHelperResolver.DeltaResult? delta = null; | ||
|
||
var configuration = RazorProjectInfoHelpers.ComputeRazorConfigurationOptions(project.AnalyzerOptions.AnalyzerConfigOptionsProvider, out var rootNamespace); |
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.
Given the null check, bool Try...(out, out)
could also be appropriate
.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs
Outdated
Show resolved
Hide resolved
…Workspace/RazorWorkspaceListenerBase.cs Co-authored-by: Dustin Campbell <dustin@teamcampbell.org>
…Workspace/RazorWorkspaceListenerBase.cs Co-authored-by: Dustin Campbell <dustin@teamcampbell.org>
…hedTagHelperResolver.DeltaResult.cs Co-authored-by: Dustin Campbell <dustin@teamcampbell.org>
…hedTagHelperResolver.DeltaResult.cs Co-authored-by: Dustin Campbell <dustin@teamcampbell.org>
…d/razor into reduce_namedpipe_chatter
Converting to draft while I think on this more |
Replaced with #10813 |
This is the start of making it so the information sent over the named pipe is less noisy. The aim is to detect if any changes need to be synchronized to razor. It still calculates and serializes the full project information; the aim is to reduce that in a later PR.