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

Sync project level info in parallel. #72976

Merged
merged 58 commits into from
Apr 12, 2024

Conversation

CyrusNajmabadi
Copy link
Member

Followup to #72975

Shaves off 5 seconds locally when syncing (from 15s to 10s).

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner April 10, 2024 22:46
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 10, 2024

if (searchingChecksumsLeft.Remove(projectStateChecksums.CompilationOptions))
result[projectStateChecksums.CompilationOptions] = projectState.CompilationOptions!;
await projectStateChecksums.FindAsync(
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 code was duplicating exactly what projectStateChecksums.FindAsync already does. We just want to be able to search projects, just without a particular project-id provided. so this now defers to that helper to do that. thanks the enum values this is easy.

@@ -22,7 +23,7 @@ namespace Microsoft.CodeAnalysis.Remote;
internal sealed partial class AssetProvider(Checksum solutionChecksum, SolutionAssetCache assetCache, IAssetSource assetSource, ISerializerService serializerService)
: AbstractAssetProvider
{
private const int PooledChecksumArraySize = 256;
private const int PooledChecksumArraySize = 1024;
Copy link
Member Author

Choose a reason for hiding this comment

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

i upped this as i was gonig over 256 a fair bit (easy when we have 900 projects in roslyn alone).


checksums.Clear();
async Task SynchronizeProjectAssetCollectionAsync<TAsset>(Func<ProjectStateChecksums, ChecksumCollection> getChecksums, CancellationToken cancellationToken)
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 almost the same as the prior method. just the primary method says "we have one checksum of a particular type per project" whereas thsi method says "we have many checksums of a particular type per project".

Note; lots of checksums will be duplicated across projects (like metadata and analyzer references) so thsi hashset dedupes a lot most of the time.


// Then sync each project's documents in parallel with each other.
foreach (var projectChecksums in allProjectChecksums)
tasks.Add(SynchronizeProjectDocumentsAsync(projectChecksums, cancellationToken));
Copy link
Contributor

Choose a reason for hiding this comment

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

cancellationToken

Any concern about a potential large increase in cancellation exceptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can cross that bridge if it happens :-)

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

await this.SynchronizeAssetsAsync<object, Dictionary<Checksum, object>>(
assetPath: AssetPath.SolutionAndTopLevelProjectsOnly,
assetPath: new(AssetPathKind.Solution | AssetPathKind.ProjectStateChecksums),
Copy link
Contributor

Choose a reason for hiding this comment

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

new

nit: could use implicit ctor

/// <summary>
/// Instance that will only look up solution-level data when searching for checksums.
/// </summary>
public static readonly AssetPath SolutionOnly = AssetPathKind.Solution;
Copy link
Contributor

Choose a reason for hiding this comment

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

SolutionOnly

nit: doesn't seem necessary. Almost all other callers just specify the AssetPathKind explicitly.

@@ -125,16 +125,17 @@ public async Task<Checksum> GetChecksumAsync(ProjectId projectId, CancellationTo
}

ChecksumCollection? frozenSourceGeneratedDocumentIdentities = null;
ChecksumsAndIds<DocumentId>? frozenSourceGeneratedDocuments = null;
ChecksumsAndIds<DocumentId>? frozenSourceGeneratedDocumentTexts = null;
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 got simplified to avoid an extra indirection and roundtrip. previously we would store the DocumentStateCheckums for the frozen SG docs. Then, OOP would ask for these, just to get the text checksum, just to call back into the host to get this. This changes to now just store the text checksums only.

@CyrusNajmabadi CyrusNajmabadi merged commit 3886db5 into dotnet:main Apr 12, 2024
27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 12, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the syncStripesParallel branch April 12, 2024 00:35
@CyrusNajmabadi
Copy link
Member Author

@jasonmalinowski For review when you get back.

@dibarbet dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants