-
Notifications
You must be signed in to change notification settings - Fork 198
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
IDocumentSnapshot finally gets ValueTasks and CancellationTokens #11002
Conversation
This commit is ginormous because I needed to thread cancellation tokens around everywhere.
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorCodeDocumentExtensions.cs
Outdated
Show resolved
Hide resolved
_computedState ??= new ComputedStateTracker(this); | ||
} | ||
} | ||
=> _computedState ??= InterlockedOperations.Initialize(ref _computedState, new ComputedStateTracker()); |
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.
Whilst I have no complaints about it, I find this change very thought provoking. Your comments about computed state tracker being possibly over-complicated is definitely a feeling I share, and I think this change is some ways makes it almost a guarantee.
All computed state trackers for a document state used to share a lock, which means two trackers for the same document snapshot could not double up on work. Of course this may never have happened, and therefore seems overcomplicated, but if it did happen then this change will mean that work is now done in parallel, with some of it being thrown away. I don't know if that is a problem, but I do know it essentially ensures that the computer state tracker is over complicated for the job is now does.
At the end of the day I don't think we'll ever know, because our RPS coverage is pretty small, and I don't have any argument against simplifying the whole thing anyway, but as I said, I find it thought provoking. We've gotten huge benefits from simplifying our project system, and making things work in sensible ways with sensible inputs, and this is definitely a change on that path. If our RPS/Speedometer test were broader I might argue for holding this off until the next PR which further simplifies the tracker, so we can see its true impact, and part of me is a little sad we can't do that. I would love to have hard numbers that actually confirm its over-engineered-ness.
In summary, I have no idea why I wrote this essay 🤷♂️
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.
Your essay is well written, and I appreciate it!
All computed state trackers for a document state used to share a lock, which means two trackers for the same document snapshot could not double up on work.
I'm a little confused and read the code differently. Perhaps I missed something?
- A
DocumentState
has exactly oneComputedStateTracker
and it is created under a lock so there will only ever be oneComputedStateTracker
created forDocumentState
. - A
DocumentSnapshot
has exactly oneDocumentState
.
Given the above, how could there ever be two trackers for the same document snapshot? Unless I'm missing something, that situation never exists because it's explicitly guarded against. That said, all of this code is a bit sketchy. So, it's perfectly reasonable that I've missed something. 😄
Now, it's true that a new DocumentState
can be handed a ComputedStateTracker
from an older DocumentState
. However, in that case, the new DocumentState
has a ComputeStateTracker
that references the lock from an old DocumentState
and no lock sharing occurs.
On a related note, there are two specific cases where a DocumentState
can reference the ComputedStateTracker
from an older DocumentState
:
- When imports change
- When the project workspace state changes.
Both of these cases are very suspicious to me because they are scenarios that I would expect the tracker to be thrown away! Unfortunately, the author didn't write any meaningful comments to justify sharing the tracker because these particular changes can be very significant, semantically. What if the ProjectWorkspaceState
has a different C# language version? What if the imports add or remove a critical using directive? In either case, the new DocumentState
will have a bad RazorCodeDocument
cached and it will take a more significant update (like changing document text) to get back to a correct state.
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.
It was those two cases I was thinking of, though I'm not sure I'd thought about it enough to realise it was only two. I agree that the old way is not necessarily correct, and as I said, I like that we're dragging Razor into a correct and somewhat predictable world.
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.
It was those two cases I was thinking of, though I'm not sure I'd thought about it enough to realise it was only two. I agree that the old way is not necessarily correct, and as I said, I like that we're dragging Razor into a correct and somewhat predictable world.
If you (@davidwengier) or @ryzngard wouldn't mind, could one or both you take a closer look at the original code in main the ComputedStateTracker
lifetime and its lock and let me know if you spot anything? I'm confident in my read in there, but I'd love another set of eyes on the code.
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.
Looking at the code here's the scenarios I see
ProjectState.With{Added, Changed, Removed}HostDocument
, will have a newerProjectWorkspaceStateVersion, DocumentCollectionVersion, ConfigurationVersion
ProjectState.WithHostProject
will have a newerProjectWorkspaceStateVersion, DocumentCollectionVersion, ConfigurationVersion
ProjectState.WithProjectWorkspaceState
will have a newerProjectWorkspaceStateVersion, DocumentCollectionVersion, ConfigurationVersion
The check itself is likely never actually valid then... The code as written now seems to be functionally the same as main
Line 159 in 7ec108e
private async Task<(RazorCodeDocument, VersionStamp)> ComputeGeneratedOutputAndVersionAsync(ProjectSnapshot project, IDocumentSnapshot document) |
.../src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/CSharpOnTypeFormattingPass.cs
Outdated
Show resolved
Hide resolved
This moves GetCSharpSyntaxTree from RazorCodeDocumentExtensions to be a static method on DocumentSnapshot. This should help keep it from being called in co-hosting scenarios.
This pull request builds on my last one (#10995), continuing to make improvements to
IDocumentSnapshot
. The key change is that all of theTask
-returning methods onIDocumentSnapshot
now returnValueTask
and takeCancellationTokens
. I've touched a lot of code to threadCancellationTokens
everywhere, but it's mostly very mechanical.There is almost certainly another pull request coming at some point after this one. Next on my list is to tackle the dreaded
ComputedStateTracker
. I've long suspected that most the complexity in that class can just be deleted, and I intend to dig into that soon.