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

Use IAsyncEnumerable in DesignerAttributeScanning as well. #64616

Merged
merged 27 commits into from
Oct 19, 2022

Conversation

CyrusNajmabadi
Copy link
Member

Followup to #64576.

[Fact]
public async Task NoDesignerTest1()
private static readonly TestComposition s_inProcessComposition = EditorTestCompositions.EditorFeatures;
private static readonly TestComposition s_outOffProcessComposition = s_inProcessComposition.WithTestHostParts(TestHost.OutOfProcess);
Copy link
Member Author

Choose a reason for hiding this comment

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

yaay, now we actually test oop for this feature.

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review October 17, 2022 23:45
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner October 17, 2022 23:45
@CyrusNajmabadi
Copy link
Member Author

@jasonmalinowski this is ready for review.


// Only bother reporting non-empty information to save an unnecessary RPC.
Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't necessary anymore. by definition, if we don't yield something, we have nothing to send over the rpc channel.

@CyrusNajmabadi CyrusNajmabadi marked this pull request as draft October 17, 2022 23:53
@CyrusNajmabadi
Copy link
Member Author

Moving back to draft. I realize we do something slightly funky here where we're mutating state in the middle of streamign the data, and we have to know exactly what both sides think teh state of the world is.

/// Keep track of the last information we reported. We will avoid notifying the host if we recompute and these
/// don't change.
/// </summary>
private readonly ConcurrentDictionary<DocumentId, (string? category, VersionStamp projectVersion)> _documentToLastReportedInformation = new();
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the remote side. it became completely stateless (which is good). there was a lot of complexity about the remote side trying to keep track of what hte host side might now and preventing updates. this was error prone and doesn't work in the IASyncEnumerable world. e.g. in remote IAE, jsut because you yielded the data, you dont' know that hte host has actually gotten it.

this moved to a cleaner model where the remote side knows nothing, and the host side keeps track.

{
var data = await ComputeDesignerAttributeDataAsync(designerCategoryType, priorityDocument, cancellationToken).ConfigureAwait(false);
if (data != null)
yield return data.Value;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is much cleaner too. we only send the data for things that actually have a category. in practice this is a tiny handful of messages in a few projects. This is different from before when we'd send info about every document, even those without a category.

@@ -58,19 +58,16 @@ public ValueTask HydrateAsync(Checksum solutionChecksum, CancellationToken cance
ImmutableArray<string> kinds,
CancellationToken cancellationToken)
{
return StreamWithSolutionAsync(solutionChecksum, SearchDocumentWorkerAsync, cancellationToken).WithJsonRpcSettings(
Copy link
Member Author

Choose a reason for hiding this comment

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

these changes were a request from Jason from teh last pr.

@@ -5,24 +5,24 @@
using System;
Copy link
Member Author

Choose a reason for hiding this comment

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

the meat of the change is in this type. This is the host-side of the feature. so this is the side that 'pulls' on the remote side to do the computation, then 'pushes' that data into the project system.

@DustinCampbell @tmeschter, ideally in teh future we can just have teh project system pull on this data using LSP, instead of this hybrid push/pull model.

foreach (var (documentId, lastReportedData) in _documentToLastReportedData)
{
if (documentId.ProjectId == projectId &&
!seenDocuments.Contains(documentId))
Copy link
Member

Choose a reason for hiding this comment

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

should we check that the document still exists before reporting a null category, or does the project system handle that

Copy link
Member Author

Choose a reason for hiding this comment

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

we check for the legacy project system (where we're on the UI thread, and where we need an ItemId). But we don't check on hte CPS case because it's inherently 'racey'. we're issueing on the BG, so there's no way for us to ever know for sure. the API is resilient to this though.

/// we can dump our cached data for them.
/// </para>
/// </summary>
private readonly AsyncBatchingWorkQueue<(CodeAnalysis.Solution? solution, DesignerAttributeData? data)> _projectSystemNotificationQueue;
Copy link
Member Author

Choose a reason for hiding this comment

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

the PS queue now gets both the data-values, and the last solution we computed against. with that solution, it can dump old data for documents that are gone.

if (!_documentToLastReportedData.TryGetValue(data.DocumentId, out var existingData) ||
existingData.Category != data.Category)
{
changedData.Add(data);
Copy link
Member

Choose a reason for hiding this comment

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

so it looks like we compare the changed docIds (aka everything that came from the dataList that wasn't null) to the last reported docIds and only trigger notifications for ones that are different which seems fine.

But where do we compare the last reported docs that are no longer present in the new data (aka the designer attribute was removed)?

Copy link
Member Author

Choose a reason for hiding this comment

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

sigh. good catch. i'm too tired.

Copy link
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.

looks good - one possible improvement suggestion

@CyrusNajmabadi CyrusNajmabadi merged commit 8ed28cd into dotnet:main Oct 19, 2022
@ghost ghost added this to the Next milestone Oct 19, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the designerAttributeStream branch October 19, 2022 20:14
if (projectId == priorityDocument?.Id.ProjectId)
continue;

await ProcessProjectAsync(client, solution.GetRequiredProject(projectId), priorityDocumentId: null, cancellationToken).ConfigureAwait(false);
Copy link
Member

@genlu genlu Oct 20, 2022

Choose a reason for hiding this comment

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

This this throwing exceptions in RPS

 Throw(StreamJsonRpc.RemoteInvocationException) Project of ID (ProjectId, #62aad7d9-16aa-4839-8e9c-7cd963aa2784 - /dev/null/inferredProject1*`0bc1d2bb-ce16-43a5-8b55-d3e19862f744) is required to accomplish the task but is not available from the solution
+ Other <<mscorlib.ni!ExceptionDispatchInfo.Throw>>
+ Other <<mscorlib.ni!TaskAwaiter.HandleNonSuccessAndDebuggerNotification>>
|+ microsoft.codeanalysis.workspaces.ni!Roslyn.Utilities.AsyncBatchingWorkQueue`2+<<AddWork>g__ContinueAfterDelay|15_1>d[Roslyn.Utilities.VoidResult,Roslyn.Utilities.VoidResult].MoveNext()
||+ Other <<mscorlib.ni!System.Threading.Tasks.Task.Finish(Boolean)>>
|| + microsoft.codeanalysis.workspaces.ni!System.Threading.Tasks.Task`1[Roslyn.Utilities.VoidResult].TrySetException(System.Object)
||  + microsoft.codeanalysis.workspaces.ni!System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[Roslyn.Utilities.VoidResult].SetException(System.Exception)
||   + microsoft.codeanalysis.workspaces.ni!Roslyn.Utilities.AsyncBatchingWorkQueue`2+<ProcessNextBatchAsync>d__17[Roslyn.Utilities.VoidResult,Roslyn.Utilities.VoidResult].MoveNext()
||    + Other <<mscorlib.ni!TaskAwaiter.HandleNonSuccessAndDebuggerNotification>>
||     + microsoft.codeanalysis.workspaces.ni!Roslyn.Utilities.AsyncBatchingWorkQueue`2+<ProcessNextBatchAsync>d__17[Roslyn.Utilities.VoidResult,Roslyn.Utilities.VoidResult].MoveNext()
||      + Other <<mscorlib.ni!Task.Finish>>
||       + microsoft.codeanalysis.workspaces.ni!System.Threading.Tasks.Task`1[Roslyn.Utilities.VoidResult].TrySetException(System.Object)
||        + microsoft.codeanalysis.workspaces.ni!System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[Roslyn.Utilities.VoidResult].SetException(System.Exception)
||         + microsoft.codeanalysis.workspaces.ni!Roslyn.Utilities.AsyncBatchingWorkQueue`1+<>c__DisplayClass2_0+<<Convert>b__0>d[Roslyn.Utilities.VoidResult].MoveNext()
||          + Other <<mscorlib.ni!TaskAwaiter.HandleNonSuccessAndDebuggerNotification>>
||           + microsoft.codeanalysis.workspaces.ni!Roslyn.Utilities.AsyncBatchingWorkQueue`1+<>c__DisplayClass2_0+<<Convert>b__0>d[Roslyn.Utilities.VoidResult].MoveNext()
||            + Other <<mscorlib.ni!System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[System.Threading.Tasks.VoidTaskResult].SetException(System.Exception)>>
||             + system.threading.tasks.extensions.ni!?
||              + microsoft.visualstudio.languageservices.ni!Microsoft.VisualStudio.LanguageServices.Implementation.DesignerAttribute.VisualStudioDesignerAttributeService+<ProcessWorkspaceChangeAsync>d__12.MoveNext()
||               + Other <<mscorlib.ni!ExceptionDispatchInfo.Throw>>
||                + microsoft.visualstudio.languageservices.ni!Microsoft.VisualStudio.LanguageServices.Implementation.DesignerAttribute.VisualStudioDesignerAttributeService+<ProcessWorkspaceChangeAsync>d__12.MoveNext()

CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Oct 20, 2022
…ttributeStream"

This reverts commit 8ed28cd, reversing
changes made to 795612c.
JoeRobich added a commit that referenced this pull request Oct 20, 2022
CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Oct 21, 2022
…esignerAttributeStream""

This reverts commit 13a67a8.
@RikkiGibson RikkiGibson modified the milestones: Next, 17.5 P1 Oct 24, 2022
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