Skip to content
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

Avoid using Document snapshot in perf-sensitive command handlers #57554

Closed
Tracked by #58717
tmat opened this issue Nov 3, 2021 · 2 comments
Closed
Tracked by #58717

Avoid using Document snapshot in perf-sensitive command handlers #57554

tmat opened this issue Nov 3, 2021 · 2 comments

Comments

@tmat
Copy link
Member

tmat commented Nov 3, 2021

Even after we move fully to LSP and OOP we anticipate having to have certain syntax based operations implemented on the client (VS in-proc) for performance reasons. For example, smart indentation. These operations should avoid accessing Document or other parts of solution snapshot since the snapshot APIs provide asynchronous operations and we want to avoid any such operations (even accidental use).

Async operations synchronously waited on from UI thread potentially introduce UI delays (the editor team has telemetry indicating that some Roslyn handlers indeed cause UI delays). GetTextSynchronously was designed to be used to avoid these delays, assuming that text parsing is fast enough to be done on UI thread. The issue at the moment is that features have access to Document and any async operation it provides, so it's easy for the implementations to not know that they are being called from a command handler and need to be synchronous. ParsedDocument constraints the implementation since it does not provide any async operations to fetch text/syntax tree.

To clearly separate the implementation of these operations from the server-side features (that have access to snapshot), we introduce an abstraction that holds on just the information necessary to implement these operations:

readonly record struct ParsedDocument(
   HostLanguageServices LanguageServices,
   SyntaxNode Root,
   SourceText Text,
   DocumentId Id,
   string? FilePath
);

The operations should be refactored to replace their usage of Document with ParsedDocument.

In addition, these synchronous operations should also avoid updating Workspace directly (e.g. TryApplyChanges). Instead, the text buffer should be updated directly as if the user made the change by typing. The workspace will be auto updated via text buffer change events. Tracked by #63574

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 3, 2021
@jinujoseph jinujoseph added Area-Performance Concept-Continuous Improvement and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 4, 2021
@jinujoseph jinujoseph added this to the 17.1 milestone Nov 4, 2021
@CyrusNajmabadi
Copy link
Member

Importantly, the above ensures that features should be able to operate potentially without asynchrony, as hte expensive (and potentially IO) work was moved out of them.

@CyrusNajmabadi
Copy link
Member

Closing out as we've effectively done this. If htere are remaining items we can track with specific issues for those areas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants