Skip to content

Have tagger wait on the OOP host having a complete compilation rather than waiting for the local host to have it.#57290

Merged
CyrusNajmabadi merged 5 commits intodotnet:mainfrom
CyrusNajmabadi:taggerOop
Oct 26, 2021
Merged

Have tagger wait on the OOP host having a complete compilation rather than waiting for the local host to have it.#57290
CyrusNajmabadi merged 5 commits intodotnet:mainfrom
CyrusNajmabadi:taggerOop

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

Several of our taggers use frozen-partial semantics in order to sync over only a portion of the workspace to OOP, and to provide potentially inaccurate initial results as early as possible. The downside here though is that that may mean that stale results could be shown in the editor indefinitely. To prevent the staleness issue, anytime we computed the stale results, we'd also kick off work to produce a full compilation for the current project, so we would know to retrigger our taggers to get non-stale data and show the user the fully correct values once available.

This worked, but had an odd quirk to it. Specifically, computation of hte tags would happen in OOP, but computation of the compilation would happen in VS. So we'd effectively be saying: ah, teh VS compilation is up to date, now let's call into OOP to compute the new results. But the call to oop would then have to then compute that same compilation.

The change here is to not have hte compilation computation occur on the VS side. Instead, the features that are doing this OOP work instead ask to retrigger when the OOP side computes the full compilation. Then, the request can go to OOP and tehn work on the compilation which is already available there.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner October 21, 2021 00:16
@ghost ghost added the Area-IDE label Oct 21, 2021
… than waiting for the local host to have it.
Copy link
Copy Markdown
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically, computation of hte tags would happen in OOP, but computation of the compilation would happen in VS. So we'd effectively be saying: ah, teh VS compilation is up to date, now let's call into OOP to compute the new results. But the call to oop would then have to then compute that same compilation.

ahh that's tricky...


namespace Microsoft.CodeAnalysis.Remote
{
internal sealed class RemoteTaggerCompilationAvailableService : BrokeredServiceBase, IRemoteTaggerCompilationAvailableService
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this should have tagger in the name - OOP doesn't 'know' about tagging so it seems weird to have it be part of the name. Maybe just IRemoteCompilationAvailableService?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup. i can do that.

var solution = await GetSolutionAsync(solutionInfo, cancellationToken).ConfigureAwait(false);
var project = solution.GetRequiredProject(projectId);

await CompilationAvailableTaggerEventSource.ComputeCompilationInCurrentProcessAsync(project, cancellationToken).ConfigureAwait(false);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the out of process component supposed to have dependencies on the editor features layer? as above it seems a little strange to have the OOP know about 'tagging'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, moved this to be more of a core service as opposed to something related to tagging.

Copy link
Copy Markdown
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm signing off, but @dibarbet asks many good questions.

@CyrusNajmabadi CyrusNajmabadi merged commit 054aaca into dotnet:main Oct 26, 2021
@ghost ghost added this to the Next milestone Oct 26, 2021
@allisonchou allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the taggerOop branch May 3, 2022 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants