Allow server to pass along a specific project-scope it wants to sync data for. #70342

CyrusNajmabadi · 2023-10-11T20:19:06Z

This information allows the host side to search far less when looking for the matches for the information the server wants.

Note: more PRs are still coming past this point.

CyrusNajmabadi · 2023-10-12T17:59:47Z

src/VisualStudio/Core/Test.Next/Remote/SerializationValidator.cs

@@ -27,7 +27,7 @@ private sealed class AssetProvider : AbstractAssetProvider
            public AssetProvider(SerializationValidator validator)
                => _validator = validator;

-            public override async ValueTask<T> GetAssetAsync<T>(Checksum checksum, CancellationToken cancellationToken)
+            public override async ValueTask<T> GetAssetAsync<T>(ProjectId? hintProject, Checksum checksum, CancellationToken cancellationToken)


can pass this 'hint' along to tell the host where to search, isntead of searching everything.

Note: in a followup PR thsi becomes more generic, so we can also scope down to the document level as well (so it's trivial to get data for things like a document, without having to do a full solution sweep).

CyrusNajmabadi · 2023-10-12T18:00:56Z

src/Workspaces/Core/Portable/Workspace/Solution/StateChecksums.cs

            // It's possible not all all our projects have checksums.  Specifically, we may have only been
            // asked to compute the checksum tree for a subset of projects that were all that a feature needed.
            if (projectState.TryGetStateChecksums(out var projectStateChecksums))
                await projectStateChecksums.FindAsync(projectState, searchingChecksumsLeft, result, cancellationToken).ConfigureAwait(false);
        }
-
-        ChecksumCollection.Find(state.AnalyzerReferences, AnalyzerReferences, searchingChecksumsLeft, result, cancellationToken);


by moving this above the projects, we don't bother searching into documents when someone is just trying to sync the solution-checksum-info in isolation.

CyrusNajmabadi · 2023-10-12T18:01:58Z

src/Workspaces/Remote/Core/AbstractAssetProvider.cs

-        var solutionChecksums = await GetAssetAsync<SolutionStateChecksums>(solutionChecksum, cancellationToken).ConfigureAwait(false);
-        var solutionAttributes = await GetAssetAsync<SolutionInfo.SolutionAttributes>(solutionChecksums.Attributes, cancellationToken).ConfigureAwait(false);
+        var solutionChecksums = await GetAssetAsync<SolutionStateChecksums>(hintProject: null, solutionChecksum, cancellationToken).ConfigureAwait(false);
+        var solutionAttributes = await GetAssetAsync<SolutionInfo.SolutionAttributes>(hintProject: null, solutionChecksums.Attributes, cancellationToken).ConfigureAwait(false);


no hints here (because this is solution info, not project info). However, with teh reordering we did in the solution-finding code, this will all be cheap. that's because we search the direct (non-project-child) info of the solution first, before recursing. So we'll always find this data in O(1) time, instead of O(solution) time.

CyrusNajmabadi · 2023-10-12T18:03:09Z

src/Workspaces/Remote/Core/AbstractAssetProvider.cs


        return SolutionInfo.Create(
            solutionAttributes.Id, solutionAttributes.Version, solutionAttributes.FilePath, projects.ToImmutableAndClear(), analyzerReferences).WithTelemetryId(solutionAttributes.TelemetryId);
    }

    public async Task<ProjectInfo?> CreateProjectInfoAsync(Checksum projectChecksum, CancellationToken cancellationToken)
    {
-        var projectChecksums = await GetAssetAsync<ProjectStateChecksums>(projectChecksum, cancellationToken).ConfigureAwait(false);
+        var projectChecksums = await GetAssetAsync<ProjectStateChecksums>(hintProject: null, projectChecksum, cancellationToken).ConfigureAwait(false);


note: this is a place where passing a project-id would be better. a followup PR accomplishes this.

CyrusNajmabadi · 2023-10-12T18:03:32Z

src/Workspaces/Remote/Core/AbstractAssetProvider.cs


-        var attributes = await GetAssetAsync<ProjectInfo.ProjectAttributes>(projectChecksums.Info, cancellationToken).ConfigureAwait(false);
+        var projectId = projectChecksums.ProjectId;
+        var attributes = await GetAssetAsync<ProjectInfo.ProjectAttributes>(hintProject: projectId, projectChecksums.Info, cancellationToken).ConfigureAwait(false);


in this PR, we don't know the projectID without this roundtrip. A future PR fixes that, so we have that info up front.

CyrusNajmabadi · 2023-10-12T18:04:48Z

src/Workspaces/Remote/Core/AbstractAssetProvider.cs

-        var serializableSourceText = await GetAssetAsync<SerializableSourceText>(documentSnapshot.Text, cancellationToken).ConfigureAwait(false);
+        var documentSnapshot = await GetAssetAsync<DocumentStateChecksums>(hintProject: projectId, documentChecksum, cancellationToken).ConfigureAwait(false);
+        var attributes = await GetAssetAsync<DocumentInfo.DocumentAttributes>(hintProject: projectId, documentSnapshot.Info, cancellationToken).ConfigureAwait(false);
+        var serializableSourceText = await GetAssetAsync<SerializableSourceText>(hintProject: projectId, documentSnapshot.Text, cancellationToken).ConfigureAwait(false);


this scopes us down to a particular project to find this data. but we still need to search the whole project to find the document info being asked for. a followup PR addresses this by allowing the doc-id to be passed along as well to narrow the search even further.

CyrusNajmabadi · 2023-10-12T18:05:09Z

src/Workspaces/Remote/Core/SolutionAssetProvider.cs

        {
            // The responsibility is on us (as per the requirements of RemoteCallback.InvokeAsync) to Complete the
            // pipewriter.  This will signal to streamjsonrpc that the writer passed into it is complete, which will
            // allow the calling side know to stop reading results.
            Exception? exception = null;
            try
            {
-                await WriteAssetsWorkerAsync(pipeWriter, solutionChecksum, checksums, cancellationToken).ConfigureAwait(false);
+                await WriteAssetsWorkerAsync(pipeWriter, solutionChecksum, hintProject, checksums, cancellationToken).ConfigureAwait(false);


a lot of hte change is just passing this data along.

CyrusNajmabadi · 2023-10-12T18:06:25Z

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.SolutionCreator.cs

@@ -477,7 +489,8 @@ private async Task<Project> UpdateDocumentAsync(TextDocument document, DocumentS
                // changed text
                if (oldDocumentChecksums.Text != newDocumentChecksums.Text)
                {
-                    var serializableSourceText = await _assetProvider.GetAssetAsync<SerializableSourceText>(newDocumentChecksums.Text, cancellationToken).ConfigureAwait(false);
+                    var serializableSourceText = await _assetProvider.GetAssetAsync<SerializableSourceText>(
+                        hintProject: document.Project.Id, newDocumentChecksums.Text, cancellationToken).ConfigureAwait(false);


a future change makes it so we can scope this down to a particular document, not jsut a project.

ToddGrun · 2023-10-12T18:54:20Z

src/Workspaces/Core/Portable/Workspace/Solution/StateChecksums.cs

+        // This ensures that when we are trying to sync the projects referenced by a SolutionStateChecksums' instance
+        // that we don't unnecessarily walk all documents looking just for those.
+
+        foreach (var (projectId, projectState) in state.ProjectStates)


state.ProjectStates

I wonder if the hintProject case would be better by just creating an ImmutableDictionary with the one entry in it and enumerating over it in the two loops here, rather than iterating over all the project states

this changes in a followup pr. i use the hint project to just index directly into state.ProjectStates.

ToddGrun · 2023-10-12T19:03:10Z

src/Workspaces/Remote/Core/AbstractAssetProvider.cs

@@ -19,28 +19,29 @@ internal abstract class AbstractAssetProvider
    /// <summary>
    /// return data of type T whose checksum is the given checksum
    /// </summary>
-    public abstract ValueTask<T> GetAssetAsync<T>(Checksum checksum, CancellationToken cancellationToken);
+    public abstract ValueTask<T> GetAssetAsync<T>(ProjectId? hintProject, Checksum checksum, CancellationToken cancellationToken);


ProjectId? hintProject,

If you see other hint data coming down the line, would it make sense for this to be a struct so everywhere piping it through wouldn't need to change?

that happens in the followup :)

ToddGrun · 2023-10-12T19:06:01Z

src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs

+/// <summary>
+/// This service provide a way to get roslyn objects from checksum
+/// </summary>
+internal sealed partial class AssetProvider(Checksum solutionChecksum, SolutionAssetCache assetCache, IAssetSource assetSource, ISerializerService serializerService)


partial

Is there another part to this class that I'm not seeing in this PR?

nm, I see that the ChecksumSynchronizer moved inside. Perhaps rename that file?

ToddGrun · 2023-10-12T19:18:36Z

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.SolutionCreator.cs

@@ -402,7 +412,8 @@ private async Task<Project> UpdateProjectInfoAsync(Project project, Checksum inf
                        lazyDocumentsToAdd ??= ImmutableArray.CreateBuilder<DocumentInfo>();

                        // we have new document added
-                        var documentInfo = await _assetProvider.CreateDocumentInfoAsync(newDocumentChecksums.Checksum, cancellationToken).ConfigureAwait(false);
+                        var documentInfo = await _assetProvider.CreateDocumentInfoAsync(
+                            project.Id, newDocumentChecksums.Checksum, cancellationToken).ConfigureAwait(false);


project.Id

nit: everywhere else seems to be name param'ing this

ToddGrun

CyrusNajmabadi added 8 commits October 10, 2023 13:10

Add project id hint

2b6b842

Add project id hint

f72b32e

Cleanup

1fab9f7

Fixes

c2b8de8

Fix test

528a01b

Fix

eb5f197

Fix

ab6e43d

Fix

bf4badd

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 11, 2023

CyrusNajmabadi and others added 5 commits October 11, 2023 13:19

Merge branch 'main' into checksumCleanup6

afe7f7a

Merge branch 'checksumCleanup5' into checksumCleanup6

0103f81

Merge branch 'checksumCleanup5' into checksumCleanup6

66dc991

Merge branch 'checksumCleanup5' into checksumCleanup6

d174bb8

Merge remote-tracking branch 'upstream/main' into checksumCleanup6

4b619d7

CyrusNajmabadi changed the title ~~WIP: More checksum work (cont1)~~ Allow server to pass along a specific project-scope it wants to sync data for. Oct 12, 2023

CyrusNajmabadi marked this pull request as ready for review October 12, 2023 17:58

CyrusNajmabadi requested a review from a team as a code owner October 12, 2023 17:58

CyrusNajmabadi commented Oct 12, 2023

View reviewed changes

ToddGrun reviewed Oct 12, 2023

View reviewed changes

ToddGrun approved these changes Oct 12, 2023

View reviewed changes

build-analysis bot mentioned this pull request Oct 12, 2023

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

arunchndr merged commit 210bd5a into dotnet:main Oct 12, 2023
19 of 24 checks passed

ghost added this to the Next milestone Oct 12, 2023

CyrusNajmabadi deleted the checksumCleanup6 branch October 12, 2023 21:34

allisonchou mentioned this pull request Oct 13, 2023

[Automated] PRs inserted in VS build main-34213.14 #70369

Closed

jjonescz modified the milestones: Next, 17.9 P1 Oct 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow server to pass along a specific project-scope it wants to sync data for. #70342

Allow server to pass along a specific project-scope it wants to sync data for. #70342

CyrusNajmabadi commented Oct 11, 2023 •

edited

Loading

CyrusNajmabadi Oct 12, 2023

CyrusNajmabadi Oct 12, 2023

CyrusNajmabadi Oct 12, 2023

CyrusNajmabadi Oct 12, 2023

CyrusNajmabadi Oct 12, 2023

CyrusNajmabadi Oct 12, 2023

CyrusNajmabadi Oct 12, 2023

CyrusNajmabadi Oct 12, 2023

ToddGrun Oct 12, 2023

CyrusNajmabadi Oct 12, 2023

ToddGrun Oct 12, 2023

CyrusNajmabadi Oct 12, 2023

ToddGrun Oct 12, 2023

ToddGrun Oct 12, 2023

ToddGrun Oct 12, 2023

CyrusNajmabadi Oct 12, 2023

ToddGrun left a comment

Allow server to pass along a specific project-scope it wants to sync data for. #70342

Allow server to pass along a specific project-scope it wants to sync data for. #70342

Conversation

CyrusNajmabadi commented Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 11, 2023 •

edited

Loading