You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When we retrieve an object for Text checksum in DocumentStateChecksums we used a trick to avoid making Find asynchronous and save Task allocations. We placed DocumentState to the result instead of the expected SourceText and retrieved the actual SourceText instance later on, when we were in async context. We can avoid the Task allocations by using ValueTask, remove this special casing and simplify SolutionAsset implementation.
Add nullable annotations.
Move methods from Microsoft.CodeAnalysis.Remote.Shared.Extensions to TestUtils as they are only used for testing.
Remove SolutionAssetManager and RemoteHostPanel. They were only used for experimenting and are no longer needed.
Would we want TextDocumentState.GetTextAsync to use ValueTask to avoid Task allocation when it completes synchronously?
The overwhelming use of this method enters through TextDocument.GetTextAsync. Since that implementation delegates (as opposed to wrapping the call in its own async method), there will be an unavoidable task allocation regardless of the return type in TextDocumentState.
FWIW, TextDocument.GetTextAsync has definitely shown up on my list of async methods that would benefit from ValueTask<T>. The only reason it isn't converted is we need to find a pattern for handling public APIs.
The overwhelming use of this method enters through TextDocument.GetTextAsync. Since that implementation delegates (as opposed to wrapping the call in its own async method), there will be an unavoidable task allocation regardless of the return type in TextDocumentState
That's true, but making TextDocumentState.GetTextAsync would at least avoid some allocations until we figure out the new public API.
This file contains hidden or 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
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.
sharwell
left a comment
When we retrieve an object for Text checksum in
DocumentStateChecksumswe used a trick to avoid making Find asynchronous and save Task allocations. We placedDocumentStateto the result instead of the expectedSourceTextand retrieved the actualSourceTextinstance later on, when we were in async context. We can avoid theTaskallocations by usingValueTask, remove this special casing and simplifySolutionAssetimplementation.Add nullable annotations.
Move methods from
Microsoft.CodeAnalysis.Remote.Shared.Extensionsto TestUtils as they are only used for testing.Remove SolutionAssetManager and RemoteHostPanel. They were only used for experimenting and are no longer needed.