Cleanup in solution-sync code. #72990
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -85,7 +85,7 @@ public async Task TestAssetSynchronization() | |||
| var assetSource = new SimpleAssetSource(workspace.Services.GetService<ISerializerService>(), map); | |||
|
|
|||
| var service = new AssetProvider(sessionId, storage, assetSource, remoteWorkspace.Services.GetService<ISerializerService>()); | |||
| await service.SynchronizeAssetsAsync<object, VoidResult>(AssetPath.FullLookupForTesting, new HashSet<Checksum>(map.Keys), callback: null, arg: default, CancellationToken.None); | |||
| await service.GetAssetsAsync<object>(AssetPath.FullLookupForTesting, new HashSet<Checksum>(map.Keys), CancellationToken.None); | |||
There was a problem hiding this comment.
we had duplicate mehtods GetAssetsAsync and SynchronizeAssetsAsync. unified on the former name.
| var projectReferences = await GetAssetsAsync<ProjectReference>(new(AssetPathKind.ProjectProjectReferences, projectId), projectChecksums.ProjectReferences, cancellationToken).ConfigureAwait(false); | ||
| var metadataReferences = await GetAssetsAsync<MetadataReference>(new(AssetPathKind.ProjectMetadataReferences, projectId), projectChecksums.MetadataReferences, cancellationToken).ConfigureAwait(false); | ||
| var analyzerReferences = await GetAssetsAsync<AnalyzerReference>(new(AssetPathKind.ProjectAnalyzerReferences, projectId), projectChecksums.AnalyzerReferences, cancellationToken).ConfigureAwait(false); | ||
| var projectReferences = await this.GetAssetsArrayAsync<ProjectReference>(new(AssetPathKind.ProjectProjectReferences, projectId), projectChecksums.ProjectReferences, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
for the helper that returns an array, the name 'Array' is now in the method name to indicate that's the case. the one without htat in the name is void, without allocating.
|
|
||
| internal static class AbstractAssetProviderExtensions | ||
| { | ||
| public static Task GetAssetsAsync<TAsset>( |
There was a problem hiding this comment.
lots of hte helpers became extensions. they don't need to be in the type itself.
| { | ||
| return assetProvider.GetAssetsAsync<TAsset, VoidResult>( | ||
| assetPath, checksums, callback: null, arg: default, cancellationToken); | ||
| } |
There was a problem hiding this comment.
several helpers that don't need the callbacks, but defer to the callback form
| } | ||
|
|
||
| public static async Task GetAssetsAsync<T, TArg>( | ||
| this AbstractAssetProvider assetProvider, AssetPath assetPath, ChecksumCollection checksums, Action<Checksum, T, TArg>? callback, TArg? arg, CancellationToken cancellationToken) |
There was a problem hiding this comment.
helpers that take a ChecksumCollection instead of a HashSet<Checksum>
| @@ -221,12 +210,6 @@ async ValueTask SynchronizeSolutionAssetsWorkerAsync() | |||
|
|
|||
| return; | |||
|
|
|||
| static void AddAll(HashSet<Checksum> checksums, ChecksumCollection checksumCollection) | |||
There was a problem hiding this comment.
not needed. we already have a method on ChecksumCollection for this.
| static AssetProvider() | ||
| { | ||
| IOUtilities.PerformIO(() => File.Delete(s_logFile)); | ||
| } |
| @@ -14,7 +14,6 @@ internal enum WellKnownSynchronizationKind | |||
| // Solution snapshot state, only referencing actual user (non-generated) documents, options, and references. | |||
| SolutionState, | |||
| ProjectState, | |||
| DocumentState, | |||
There was a problem hiding this comment.
this is no longer synced to/from oop. instead, we just inline it's two checkums (attributes and text checksums) to the projectstatechecksum.
There was a problem hiding this comment.
this cuts the total number of calls form oop to the host by a third.
| } | ||
|
|
||
| async Task SynchronizeProjectDocumentsAsync(ProjectStateChecksums projectChecksums) |
There was a problem hiding this comment.
this moved to the base class. it's used in multiple places now. both when seeign that a project's documents changed, and also when doing hte initial bulk sync and makigna ProjectInfo.
…roslyn into syncStripesTypes
| @@ -52,7 +52,7 @@ static SolutionAssetCache() | |||
| /// </summary> | |||
| private readonly TimeSpan _gcAfterTimeSpan; | |||
|
|
|||
| private readonly ConcurrentDictionary<Checksum, Entry> _assets = new(concurrencyLevel: 4, capacity: 10); | |||
| private readonly ConcurrentDictionary<Checksum, Entry> _assets = new(); | |||
There was a problem hiding this comment.
We're on .net core. we don't want or need to artificially limit this dictionary. esp. when we want to do all this concurrent work.
| projects.Add(await CreateProjectInfoAsync(projectId, projectChecksum, cancellationToken).ConfigureAwait(false)); | ||
| // Fetch all the project state checksums up front. That allows gettign all the data in a single call, and | ||
| // enables parallel fetching of the projects below. | ||
| using var _1 = ArrayBuilder<ProjectStateChecksums>.GetInstance(solutionChecksums.Projects.Length, out var allProjectStateChecksums); |
There was a problem hiding this comment.
i could likely make this into the array of projectinfo tasks and populate it as we're reading back the ProjectStateChecksums.
| @@ -78,3 +79,63 @@ public bool MoveNext() | |||
| => (_checksumsAndIds.Checksums.Children[_index], _checksumsAndIds.Ids[_index]); | |||
| } | |||
| } | |||
|
|
|||
| internal readonly struct DocumentChecksumsAndIds | |||
There was a problem hiding this comment.
Maybe ChecksumsAndIdsis still just there in use cases where the caller doesn't need all the data?
|
@jasonmalinowski For review when you get back. |
One more pass after all the recent refactorings.