Remove double tree traversal in checksum syncing. #70302
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -10,7 +10,7 @@ namespace Microsoft.CodeAnalysis.Remote | |||
| /// <summary> | |||
| /// Represents a part of solution snapshot along with its checksum. | |||
| /// </summary> | |||
| internal readonly struct SolutionAsset | |||
| internal sealed class SolutionAsset | |||
There was a problem hiding this comment.
this moved from a product concept to a test-only concept.
|
|
||
| public async Task<T> GetValueAsync<T>(Checksum checksum) | ||
| { | ||
| var data = await GetRequiredAssetAsync(checksum).ConfigureAwait(false); |
There was a problem hiding this comment.
this is all test code, which expects to always find these values. so it's ok for it to put in these contract calls and requirements.
| var data = await scope.GetTestAccessor().GetAssetAsync(checksum, CancellationToken.None).ConfigureAwait(false); | ||
| Contract.ThrowIfNull(data); | ||
| return new(checksum, data); | ||
| } |
There was a problem hiding this comment.
same. also tests.
| Assert.Equal(1, solutionObject.Projects.Count); | ||
| await validator.VerifySnapshotInServiceAsync(validator.ToProjectObjects(solutionObject.Projects)[0], 0, 0, 0, 0, 0).ConfigureAwait(false); |
There was a problem hiding this comment.
this will be explained below.
There was a problem hiding this comment.
note: this was a move. it was previously a product concept, but i made it into a test-only concept.
| @@ -39,7 +39,6 @@ IEnumerator IEnumerable.GetEnumerator() | |||
|
|
|||
| public void AddAllTo(HashSet<Checksum> checksums) | |||
| { | |||
| checksums.AddIfNotNullChecksum(this.Checksum); | |||
There was a problem hiding this comment.
will also be explained below.
| if (searchingChecksumsLeft.Remove(Projects.Checksum)) | ||
| { | ||
| result[Projects.Checksum] = Projects; | ||
| } |
There was a problem hiding this comment.
this it he meat of the change. The prior logic was simply crazy, and deeply complex confusing. This it the logic that says "hey... you give me a checksum, and i will find the corresponding real object associated with it in the actual solution/project/document state.
except, lines like thsi were basically saying "oh... your checksum itself refers to a checksum object (like a SolutionChecksums/ProjectChecksums/DocumentChecksums instance), so i'll add my own checksum child back into the result.
This was one of the source of multiple passes and expensiveness with checksums. When looking up project info, we'd both recurse into child projects as real 'Project' instances, and then again as checksum instances. Insanity.
All of htat is gone now. we just pass in checksums, and we get real workspace objects back out.
There was a problem hiding this comment.
this is also what affected tests. which were testing a non-real scenario of asking for these checksums, then validating the checksum objects that were returned. all bogus things that don't reflect real scenarios.
There was a problem hiding this comment.
Is there an incoming PR that will make it so that this doesn't do the depth first search? (as you mentioned in the office the other day)
There was a problem hiding this comment.
Yes. That comes in a follow-up
| Debug.Assert(asset.Kind != WellKnownSynchronizationKind.Null, "We should not be sending null assets"); | ||
| writer.WriteInt32((int)asset.Kind); | ||
| var kind = asset.GetWellKnownSynchronizationKind(); | ||
| Contract.ThrowIfTrue(kind == WellKnownSynchronizationKind.Null); |
There was a problem hiding this comment.
note: we have this contract virtually everywhere. But i'm still finding and stamping out code that is resilient to null being passed along.
|
|
||
| await validator.VerifySynchronizationObjectInServiceAsync(solutionSyncObject).ConfigureAwait(false); | ||
|
|
||
| var solutionObject = await validator.GetValueAsync<SolutionStateChecksums>(checksum).ConfigureAwait(false); | ||
| await validator.VerifyChecksumInServiceAsync(solutionObject.Attributes, WellKnownSynchronizationKind.SolutionAttributes).ConfigureAwait(false); | ||
|
|
||
| var projectsSyncObject = await scope.GetTestAccessor().GetAssetAsync(solutionObject.Projects.Checksum, CancellationToken.None).ConfigureAwait(false); | ||
| await validator.VerifySynchronizationObjectInServiceAsync(projectsSyncObject).ConfigureAwait(false); |
There was a problem hiding this comment.
this will be explained below.
| internal enum WellKnownSynchronizationKind | ||
| { | ||
| Null, |
There was a problem hiding this comment.
removing this concept entirely. it was not allowed, and we had so many confusing checks everywhere around it.
|
|
||
| foreach (var (checksum, value) in resultPool.Object) | ||
| { | ||
| result[checksum] = new SolutionAsset(checksum, value); |
There was a problem hiding this comment.
no need to lookup everything first as objects, then wrap as SolutinoAssets. all that wrapping/checking is now in the test layer only.
CyrusNajmabadi
changed the title
Can do in followup |
Can definitely tweak in follow-up:-) |
ghost
added this to the
|
@ToddGrun i couldn't figure out what code a few of your comments were referring to. Could you clarify? I can incorporate the missing feedback into the next pr. thanks! |
|
@ToddGrun still not clear what locations those refer to :-D |
| Dictionary<Checksum, object?> result, | ||
| CancellationToken cancellationToken) | ||
| Dictionary<Checksum, object> result, | ||
| CancellationToken cancellationToken) where T : class | ||
| { | ||
| Contract.ThrowIfFalse(values.Count == checksums.Children.Length); |
There was a problem hiding this comment.
Github doesn't seem to allow comments in sections of the file that weren't changed? For the two comments that were unclear, they were both in the loop below and were a bit nit-picky, so I don't care too much if they aren't taken:
- No need to check the cancellation token in the loop, nothing expensive going on in there.
- The .Count check could just be done inside the .Remove condition as it's status can't change unless the Remove condition succeeds.
There was a problem hiding this comment.
Fair. I can revise!
Note: more PRs to come. Just trying to break things into easy pieces.