Further checksum syncing cleanup work. #70341

CyrusNajmabadi · 2023-10-11T20:18:41Z

Primary changes here are about including more information in our rich state-checksum objects (whcih are not checksums themselves) to avoid unnecessary round-trips from server to host, and also to make certain syncing methods more strongly typed to prevent passing in random innapropriate checksums into them.

More changes to come. Just trying to keep things bite-sized.

CyrusNajmabadi · 2023-10-12T05:27:05Z

src/VisualStudio/Core/Test.Next/Services/AssetProviderTests.cs

@@ -136,7 +136,7 @@ public async Task TestProjectSynchronization()
            var assetSource = new SimpleAssetSource(workspace.Services.GetService<ISerializerService>(), map);

            var service = new AssetProvider(sessionId, storage, assetSource, remoteWorkspace.Services.GetService<ISerializerService>());
-            await service.SynchronizeProjectAssetsAsync(new HashSet<Checksum> { await project.State.GetChecksumAsync(CancellationToken.None) }, CancellationToken.None);
+            await service.SynchronizeProjectAssetsAsync(await project.State.GetStateChecksumsAsync(CancellationToken.None), CancellationToken.None);


made this a more strongly tyepd overload. it now takes ProjectStateChecksums instead of a random checksum instance.

CyrusNajmabadi · 2023-10-12T05:27:37Z

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

@@ -69,6 +69,7 @@ private async Task<ProjectStateChecksums> ComputeChecksumsAsync(CancellationToke
                    var analyzerConfigDocumentChecksums = new ChecksumCollection(await analyzerConfigDocumentChecksumTasks.WhenAll().ConfigureAwait(false));

                    return new ProjectStateChecksums(
+                        this.Id,


added projectid to the projectstatechecksums. this avoids a lot of roundtrips to jsut go from the checksum object to get the info checksum to get the info to get the id.

CyrusNajmabadi · 2023-10-12T05:31:58Z

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

            using var _ = ArrayBuilder<ValueTuple<Checksum, T>>.GetInstance(checksums.Count, out var list);
            foreach (var checksum in checksums)
-                list.Add(ValueTuple.Create(checksum, await GetAssetAsync<T>(checksum, cancellationToken).ConfigureAwait(false)));
+                list.Add(ValueTuple.Create(checksum, GetRequiredAsset<T>(checksum)));


a lot of these calls to "get the asset asycnhronously" became "get teh required asset" since they are called directly after the call to actually synchronize the asset. This allows us to make stronger assertions that the data must be present, and that it's the prior syncronization call taht is fetching the items, and not this potential GetASsetAsync call that is doing it.

CyrusNajmabadi · 2023-10-12T05:32:08Z

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

@@ -100,7 +91,7 @@ public async ValueTask SynchronizeSolutionAssetsAsync(Checksum solutionChecksum,
            }
        }

-        public async ValueTask SynchronizeProjectAssetsAsync(HashSet<Checksum> projectChecksums, CancellationToken cancellationToken)
+        public async ValueTask SynchronizeProjectAssetsAsync(ProjectStateChecksums projectChecksums, CancellationToken cancellationToken)


made strongly typed.

CyrusNajmabadi · 2023-10-12T05:32:26Z

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


        public async ValueTask SynchronizeAssetsAsync(HashSet<Checksum> checksums, CancellationToken cancellationToken)
        {
            using (await s_gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false))
            {
-                await SynchronizeAssets_NoLockAsync(checksums, cancellationToken).ConfigureAwait(false);
+                // get children of solution checksum object at once
+                await _assetProvider.SynchronizeAssetsAsync(checksums, cancellationToken).ConfigureAwait(false);


inlined method.

CyrusNajmabadi · 2023-10-12T05:34:09Z

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

+            foreach (var project in solutionChecksumObject.Projects)
+            {
+                var projectStateChecksums = _assetProvider.GetRequiredAsset<ProjectStateChecksums>(project);
+                await SynchronizeProjectAssetsAsync(projectStateChecksums, cancellationToken).ConfigureAwait(false);


all this became much more strongly typed. the solution checksum points at the proejct checksums. from the project checksums we retrieve the project-state-checkum objects. which we can then use to synchronize that project.

i'm trying to make things not just take generic checksums, making it clearer what data needs to be passed to what.

Furthermore, instead of blocking the entire operation while doing a sync. We can now interleave syncing a project's worth of info at a time.

CyrusNajmabadi · 2023-10-12T05:36:19Z

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

-                var oldMap = await GetProjectMapAsync(solution, oldChecksums, cancellationToken).ConfigureAwait(false);
-                var newMap = await GetProjectMapAsync(_assetProvider, newChecksums, cancellationToken).ConfigureAwait(false);
+                await PopulateOldProjectMapAsync().ConfigureAwait(false);
+                await PopulateNewProjectMapAsync(this).ConfigureAwait(false);


inlined these helper methods to now be local functions in this main function. moved to pooled dictionaries as well to help with allocation overhead.

CyrusNajmabadi · 2023-10-12T05:37:02Z

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

+                    var projectStateChecksums = await @this._assetProvider.GetAssetsAsync<ProjectStateChecksums>(news.Object, cancellationToken).ConfigureAwait(false);
+
+                    foreach (var (_, projectStateChecksum) in projectStateChecksums)
+                        newMap.Add(projectStateChecksum.ProjectId, projectStateChecksum);


this code is much more efficient than what is is replacing. because the projectStateChecksums exposes a .ProjectId now, we avoid a pointless roundtrip between the server and the host.

CyrusNajmabadi · 2023-10-12T05:40:31Z

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

            }
        }

-        public async ValueTask SynchronizeProjectAssetsAsync(HashSet<Checksum> projectChecksums, CancellationToken cancellationToken)
+        public async ValueTask SynchronizeProjectAssetsAsync(ProjectStateChecksums projectChecksum, CancellationToken cancellationToken)


more strongly typed. much clearer about what to pass.

CyrusNajmabadi · 2023-10-12T05:41:00Z

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

            }
        }

-        private async ValueTask SynchronizeProjectAssets_NoLockAsync(IReadOnlyCollection<Checksum> projectChecksums, CancellationToken cancellationToken)
+        private async ValueTask SynchronizeProjectAssets_NoLockAsync(ProjectStateChecksums projectChecksum, CancellationToken cancellationToken)


unlike before, this syncs just one project. the caller of this is reponsible for handling multiple projects if that is relevant.

CyrusNajmabadi · 2023-10-12T05:41:36Z

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

+                    {
+                        var projectChecksums = await projectState.GetStateChecksumsAsync(cancellationToken).ConfigureAwait(false);
+                        if (olds.Object.Contains(projectChecksums.Checksum))
+                            oldMap.Add(projectId, projectChecksums);


same logic as before, just inlined.

CyrusNajmabadi · 2023-10-12T05:42:01Z

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

@@ -252,11 +256,11 @@ private async Task<Solution> UpdateProjectAsync(Project project, ProjectStateChe
                    project = await UpdateDocumentsAsync(
                        project,
                        newProjectChecksums,
-                        project.State.DocumentStates.States.Values,
+                        project.State.DocumentStates,


made this method strongly typed, which avoids unnecessary IEnumerable allocs.

CyrusNajmabadi · 2023-10-12T05:42:11Z

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

-                        (solution, documents) => solution.AddDocuments(documents),
-                        (solution, documentIds) => solution.RemoveDocuments(documentIds),
+                        static (solution, documents) => solution.AddDocuments(documents),
+                        static (solution, documentIds) => solution.RemoveDocuments(documentIds),


made explicit that these do not alloc lambdas.

CyrusNajmabadi · 2023-10-12T05:42:29Z

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

+                        var documentChecksums = await state.GetStateChecksumsAsync(cancellationToken).ConfigureAwait(false);
+                        if (olds.Object.Contains(documentChecksums.Checksum))
+                            oldMap.Add(state.Id, documentChecksums);
+                    }


this is the same logic as before. just inliend.

CyrusNajmabadi · 2023-10-12T05:42:49Z

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

+                    var documentStateChecksums = await @this._assetProvider.GetAssetsAsync<DocumentStateChecksums>(news.Object, cancellationToken).ConfigureAwait(false);
+
+                    foreach (var (_, documentStateChecksum) in documentStateChecksums)
+                        newMap.Add(documentStateChecksum.DocumentId, documentStateChecksum);


this is much more efficient than before. similar to projects, this avoids an unnecessary round trip to determine document ids.

CyrusNajmabadi · 2023-10-12T05:43:16Z

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

-                foreach (var documentChecksum in documentChecksums)
-                    infoChecksums.Add(documentChecksum.Item2.Info);
-
-                var infos = await assetProvider.GetAssetsAsync<DocumentInfo.DocumentAttributes>(infoChecksums, cancellationToken).ConfigureAwait(false);


this was the round-trip just for the purpose of getting a doc id.

CyrusNajmabadi · 2023-10-12T05:43:45Z

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

-                foreach (var projectChecksum in projectChecksums)
-                    infoChecksums.Add(projectChecksum.Item2.Info);
-
-                var infos = await assetProvider.GetAssetsAsync<ProjectInfo.ProjectAttributes>(infoChecksums, cancellationToken).ConfigureAwait(false);


this was a round trip just to get project ids.

CyrusNajmabadi · 2023-10-12T05:44:14Z

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

-        checksums.AddIfNotNullChecksum(this.Checksum);
-        checksums.AddIfNotNullChecksum(this.Info);
-        checksums.AddIfNotNullChecksum(this.Text);
-    }


only used in tests. moved there. will do the same with ProjectStateChecksums in a later PR.

CyrusNajmabadi · 2023-10-12T05:49:42Z

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

-                // third and last get direct children for all projects and documents in the solution 
-                await SynchronizeProjectAssets_NoLockAsync(solutionChecksumObject.Projects, cancellationToken).ConfigureAwait(false);
+            // third and last get direct children for all projects and documents in the solution 
+            foreach (var project in solutionChecksumObject.Projects)


lifted the loop out of the SynchronizeProjectAssets_NoLockAsync helper. this makes it easy to sync just a project at a time. note: the next PR ensures that when we go to sync a particular project that the other side can do that work efficiently (without having to scan the entire solution for the data it needs).

CyrusNajmabadi · 2023-10-12T05:51:02Z

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

+
+                async Task PopulateOldDocumentMapAsync()
+                {
+                    foreach (var (_, state) in existingTextDocumentStates.States)


by passing in TextDocumentStates<T> this becomes a no-alloc enumeration.

CyrusNajmabadi · 2023-10-12T05:51:18Z

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

@@ -497,84 +520,6 @@ private async Task<TextDocument> UpdateDocumentInfoAsync(TextDocument document,
                return document;
            }

-            private static async ValueTask<Dictionary<DocumentId, DocumentStateChecksums>> GetDocumentMapAsync(AssetProvider assetProvider, HashSet<Checksum> documents, CancellationToken cancellationToken)
-            {
-                var map = new Dictionary<DocumentId, DocumentStateChecksums>();


wasteful non-pooled dicitonary.

ToddGrun · 2023-10-12T13:37:44Z

Made a first pass through (pre-coffee) and it seems to make sense. Will make another pass through post-coffee and see if it still makes sense.

ToddGrun · 2023-10-12T15:48:06Z

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

@@ -69,6 +69,7 @@ private async Task<ProjectStateChecksums> ComputeChecksumsAsync(CancellationToke
                    var analyzerConfigDocumentChecksums = new ChecksumCollection(await analyzerConfigDocumentChecksumTasks.WhenAll().ConfigureAwait(false));

                    return new ProjectStateChecksums(
+                        this.Id,


this.Id,

General question about something not changed in this PR:

I see SelectAsArray done above, which I understand gives back an ImmutableArray that allows struct based enumeration. However, that array did require a heap allocation. It seems to me that if you know the collection will only be enumerated once locally (as in this case), it's better to have a heap based enumerator allocated than an ImmutableArray of a potentially large size.

Of course, that assumption could be broken and then you end up doing multiple enumerator allocations, but that doesn't seem too likely and too costly if it does happen. Another option if repeated enumerations is a concern would be an ArrayBuilder based SelectAs method, as that could have the benefit of no-allocation and struct based enumeration.

Or maybe I'm just overthinking this.

ToddGrun · 2023-10-12T16:13:58Z

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

-        private async ValueTask CollectProjectStateChecksumsAsync(HashSet<Checksum> set, IReadOnlyCollection<Checksum> checksums, CancellationToken cancellationToken)
-        {
-            foreach (var checksum in checksums)
+            void CollectChecksumChildren(ChecksumSynchronizer @this, ChecksumCollection collection)


ChecksumSynchronizer @this,

Curious why you passed this in

ToddGrun

CyrusNajmabadi added 15 commits October 9, 2023 16:56

in progress

eb2c93a

Pass a more strongly typed checksum

540ab5b

Inline methods

6c1db43

Inline method

05417db

Make generic

d6bafab

Inline methods

af89c81

Inline methods

05dc042

Primary constructor

6e06f55

fix

7459238

Merge branch 'checksumCleanup2' into checksumCleanup4

03278c9

Simplify by passing along id

1b8cac8

Simplify by passing along id

f95ee3d

Move method to test layer

60b0e49

Fix

b002a22

Avoid allocation

ebf9d7a

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

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

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

Open

3 tasks

CyrusNajmabadi and others added 2 commits October 11, 2023 18:01

Merge branch 'checksumCleanup2' into checksumCleanup5

57b8f38

Merge branch 'main' into checksumCleanup5

23f690a

CyrusNajmabadi commented Oct 12, 2023

View reviewed changes

Fix

61b9138

CyrusNajmabadi commented Oct 12, 2023

View reviewed changes

delete

4247354

CyrusNajmabadi commented Oct 12, 2023

View reviewed changes

CyrusNajmabadi changed the title ~~WIP: More checksum work.~~ Further checksum syncing cleanup work. Oct 12, 2023

CyrusNajmabadi marked this pull request as ready for review October 12, 2023 05:44

CyrusNajmabadi requested a review from a team as a code owner October 12, 2023 05:44

inline

e048c88

CyrusNajmabadi commented Oct 12, 2023

View reviewed changes

ToddGrun reviewed Oct 12, 2023

View reviewed changes

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

454975a

ToddGrun reviewed Oct 12, 2023

View reviewed changes

ToddGrun approved these changes Oct 12, 2023

View reviewed changes

Fix

d363a08

CyrusNajmabadi enabled auto-merge October 12, 2023 16:24

CyrusNajmabadi merged commit 348a4bf into dotnet:main Oct 12, 2023
24 checks passed

ghost added this to the Next milestone Oct 12, 2023

CyrusNajmabadi deleted the checksumCleanup5 branch October 12, 2023 17:56

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

Further checksum syncing cleanup work. #70341

Further checksum syncing cleanup work. #70341

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

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

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 commented Oct 12, 2023

ToddGrun Oct 12, 2023

ToddGrun Oct 12, 2023

ToddGrun left a comment

Further checksum syncing cleanup work. #70341

Further checksum syncing cleanup work. #70341

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

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 commented Oct 12, 2023

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