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.

Sign up for GitHub

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

Remove the BranchId concept from teh workspace #57132

Merged
merged 16 commits into from
Oct 25, 2021
Merged

Remove the BranchId concept from teh workspace #57132

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0199590
Remove branch-ids from teh workspace
CyrusNajmabadi Oct 13, 2021
2715c69
Flow downwards
CyrusNajmabadi Oct 13, 2021
d6001fc
inline
CyrusNajmabadi Oct 13, 2021
6812155
revert
CyrusNajmabadi Oct 13, 2021
d1f781c
Delete validaiton
CyrusNajmabadi Oct 13, 2021
9aae5f7
Simplify
CyrusNajmabadi Oct 13, 2021
7782d4b
Fixup
CyrusNajmabadi Oct 13, 2021
effa0bf
moved cached metadata refs to instance state
CyrusNajmabadi Oct 14, 2021
d83c686
Temp
CyrusNajmabadi Oct 14, 2021
fea7fc9
Temp
CyrusNajmabadi Oct 14, 2021
ff2362b
Move skeleton refs to ProjectState
CyrusNajmabadi Oct 14, 2021
7155386
Merge branch 'main' into branchId3
CyrusNajmabadi Oct 22, 2021
19748d1
revert'
CyrusNajmabadi Oct 22, 2021
57c7932
Merge remote-tracking branch 'upstream/main' into branchId3
CyrusNajmabadi Oct 23, 2021
e3f9cbc
Add comment
CyrusNajmabadi Oct 25, 2021
48c9652
Simplify since we're under a lock
CyrusNajmabadi Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 21 additions & 47 deletions src/Features/Core/Portable/Workspace/CompileTimeSolutionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -48,16 +49,13 @@ public Factory()

private readonly object _gate = new();

/// <summary>
/// Cached compile time solution corresponding to the <see cref="Workspace.PrimaryBranchId"/>
/// </summary>
private (int DesignTimeSolutionVersion, BranchId DesignTimeSolutionBranch, Solution CompileTimeSolution)? _primaryBranchCompileTimeCache;

/// <summary>
/// Cached compile time solution for a forked branch. This is used primarily by LSP cases where
/// we fork the workspace solution and request diagnostics for the forked solution.
/// </summary>
private (int DesignTimeSolutionVersion, BranchId DesignTimeSolutionBranch, Solution CompileTimeSolution)? _forkedBranchCompileTimeCache;
Copy link
Member Author

Choose a reason for hiding this comment

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

@NTaylorMullen instead of a version+branch pointing to the CompileTimeSolution, i instead just use a CWT mapping teh actual designTimeSolution to the compileTimeSolution. As a CWT it will be released whenever people stop holding onto this solution.

Note: i checked with @dibarbet and for LSP as long as edits aren't coming in, we'll have the same solution instance, so thsi CWT should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really appreciate you checking the LSP side of this too ❤️

#if NETCOREAPP
private readonly ConditionalWeakTable<Solution, Solution> _designTimeToCompileTimeSoution = new();
#else
// Framework lacks both a .Clear() method. So for Framework we simulate that by just overwriting this with a
Copy link
Contributor

Choose a reason for hiding this comment

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

Food for thought for a different PR: Is this a potential worthy data structure for us to eventually have? Something like a ThuperDuperConditionalWeakTable<T,U> that has the #if directives internally and exposes a Clear, AddOrUpdate etc. for Framework?

// new instance. This happens under a lock, so everyone sees a consistent dictionary.
private ConditionalWeakTable<Solution, Solution> _designTimeToCompileTimeSoution = new();
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

sadness. Framework lacks a 'clear' method on CWT.


public CompileTimeSolutionProvider(Workspace workspace)
{
Expand All @@ -67,8 +65,11 @@ public CompileTimeSolutionProvider(Workspace workspace)
{
lock (_gate)
{
_primaryBranchCompileTimeCache = null;
_forkedBranchCompileTimeCache = null;
#if NETCOREAPP
_designTimeToCompileTimeSoution.Clear();
#else
_designTimeToCompileTimeSoution = new();
#endif
}
}
};
Expand All @@ -82,10 +83,8 @@ public Solution GetCompileTimeSolution(Solution designTimeSolution)
{
lock (_gate)
{
var cachedCompileTimeSolution = GetCachedCompileTimeSolution(designTimeSolution);

// Design time solution hasn't changed since we calculated the last compile-time solution:
if (cachedCompileTimeSolution != null)
if (_designTimeToCompileTimeSoution.TryGetValue(designTimeSolution, out var cachedCompileTimeSolution) &&
cachedCompileTimeSolution != null)
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
{
return cachedCompileTimeSolution;
}
Expand Down Expand Up @@ -123,42 +122,17 @@ public Solution GetCompileTimeSolution(Solution designTimeSolution)
.RemoveAnalyzerConfigDocuments(configIdsToRemove.ToImmutable())
.RemoveDocuments(documentIdsToRemove.ToImmutable());

UpdateCachedCompileTimeSolution(designTimeSolution, compileTimeSolution);
#if NETCOREAPP
_designTimeToCompileTimeSoution.AddOrUpdate(designTimeSolution, compileTimeSolution);
Copy link
Member

Choose a reason for hiding this comment

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

We're under a lock here, is it possible for this to ever race with something else given we checked earlier?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope. not possible. have rewritten with the understanding that this is a lock.

#else
_designTimeToCompileTimeSoution.Remove(designTimeSolution);
_designTimeToCompileTimeSoution.Add(designTimeSolution, compileTimeSolution);
#endif

return compileTimeSolution;
}
}

private Solution? GetCachedCompileTimeSolution(Solution designTimeSolution)
{
// If the design time solution is for the primary branch, retrieve the last cached solution for it.
// Otherwise this is a forked solution, so retrieve the last forked compile time solution we calculated.
var cachedCompileTimeSolution = designTimeSolution.BranchId == _workspace.PrimaryBranchId ? _primaryBranchCompileTimeCache : _forkedBranchCompileTimeCache;

// Verify that the design time solution has not changed since the last calculated compile time solution and that
// the design time solution branch matches the branch of the design time solution we calculated the compile time solution for.
if (cachedCompileTimeSolution != null
&& designTimeSolution.WorkspaceVersion == cachedCompileTimeSolution.Value.DesignTimeSolutionVersion
&& designTimeSolution.BranchId == cachedCompileTimeSolution.Value.DesignTimeSolutionBranch)
{
return cachedCompileTimeSolution.Value.CompileTimeSolution;
}

return null;
}

private void UpdateCachedCompileTimeSolution(Solution designTimeSolution, Solution compileTimeSolution)
{
if (designTimeSolution.BranchId == _workspace.PrimaryBranchId)
{
_primaryBranchCompileTimeCache = (designTimeSolution.WorkspaceVersion, designTimeSolution.BranchId, compileTimeSolution);
}
else
{
_forkedBranchCompileTimeCache = (designTimeSolution.WorkspaceVersion, designTimeSolution.BranchId, compileTimeSolution);
}
}

// Copied from
// https://github.com/dotnet/sdk/blob/main/src/RazorSdk/SourceGenerators/RazorSourceGenerator.Helpers.cs#L32
private static string GetIdentifierFromPath(string filePath)
Expand Down
3 changes: 0 additions & 3 deletions src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,6 @@ private static async Task VerifySolutionUpdate(
Assert.IsAssignableFrom<RemoteWorkspace>(recoveredSolution.Workspace);
var primaryWorkspace = recoveredSolution.Workspace;
Assert.Equal(solutionChecksum, await recoveredSolution.State.GetChecksumAsync(CancellationToken.None));
Assert.Same(primaryWorkspace.PrimaryBranchId, recoveredSolution.BranchId);

// get new solution
var newSolution = newSolutionGetter(solution);
Expand All @@ -784,14 +783,12 @@ private static async Task VerifySolutionUpdate(
var recoveredNewSolution = await remoteWorkspace.GetSolutionAsync(assetProvider, newSolutionChecksum, fromPrimaryBranch: false, workspaceVersion: -1, projectId: null, CancellationToken.None);

Assert.Equal(newSolutionChecksum, await recoveredNewSolution.State.GetChecksumAsync(CancellationToken.None));
Assert.NotSame(primaryWorkspace.PrimaryBranchId, recoveredNewSolution.BranchId);

// do same once updating primary workspace
await remoteWorkspace.UpdatePrimaryBranchSolutionAsync(assetProvider, newSolutionChecksum, solution.WorkspaceVersion + 1, CancellationToken.None);
var third = await remoteWorkspace.GetSolutionAsync(assetProvider, newSolutionChecksum, fromPrimaryBranch: false, workspaceVersion: -1, projectId: null, CancellationToken.None);

Assert.Equal(newSolutionChecksum, await third.State.GetChecksumAsync(CancellationToken.None));
Assert.Same(primaryWorkspace.PrimaryBranchId, third.BranchId);

newSolutionValidator?.Invoke(recoveredNewSolution);
}
Expand Down
2 changes: 0 additions & 2 deletions src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ public static async Task PrecalculateAsync(Document document, CancellationToken

using (Logger.LogBlock(FunctionId.SyntaxTreeIndex_Precalculate, cancellationToken))
{
Debug.Assert(document.IsFromPrimaryBranch());
Copy link
Member Author

Choose a reason for hiding this comment

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

not an unreasonable assert. we would prefer some assurances that our precalculation step is only running on the non-forked solution of some workspace. However, if that turns out to not be the case, it's not the end of the world as this ensure subsystem is checksum'ed based anyways.


var checksum = await GetChecksumAsync(document, cancellationToken).ConfigureAwait(false);

// Check if we've already created and persisted the index for this document.
Expand Down
3 changes: 0 additions & 3 deletions src/Workspaces/Core/Portable/Shared/Extensions/DocumentExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ namespace Microsoft.CodeAnalysis.Shared.Extensions
{
internal static partial class DocumentExtensions
{
public static bool IsFromPrimaryBranch(this Document document)
=> document.Project.Solution.BranchId == document.Project.Solution.Workspace.PrimaryBranchId;

public static async ValueTask<SyntaxTreeIndex> GetSyntaxTreeIndexAsync(this Document document, CancellationToken cancellationToken)
{
var result = await SyntaxTreeIndex.GetIndexAsync(document, loadOnly: false, cancellationToken).ConfigureAwait(false);
Expand Down
3 changes: 0 additions & 3 deletions src/Workspaces/Core/Portable/Shared/Extensions/ProjectExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@ namespace Microsoft.CodeAnalysis.Shared.Extensions
{
internal static partial class ProjectExtensions
{
public static bool IsFromPrimaryBranch(this Project project)
=> project.Solution.BranchId == project.Solution.Workspace.PrimaryBranchId;

internal static Project WithSolutionOptions(this Project project, OptionSet options)
=> project.Solution.WithOptions(options).GetProject(project.Id)!;

Expand Down
28 changes: 0 additions & 28 deletions src/Workspaces/Core/Portable/Workspace/Solution/BranchId.cs
View file
Open in desktop

This file was deleted.

26 changes: 7 additions & 19 deletions src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,12 @@ namespace Microsoft.CodeAnalysis
internal class MetadataOnlyReference
{
// version based cache
private static readonly ConditionalWeakTable<BranchId, ConditionalWeakTable<ProjectId, MetadataOnlyReferenceSet>> s_cache
= new();
private static readonly ConditionalWeakTable<SolutionState, ConditionalWeakTable<ProjectId, MetadataOnlyReferenceSet>> s_cache = new();

// snapshot based cache
private static readonly ConditionalWeakTable<Compilation, MetadataOnlyReferenceSet> s_snapshotCache
= new();
private static readonly ConditionalWeakTable<Compilation, MetadataOnlyReferenceSet> s_snapshotCache = new();

private static readonly ConditionalWeakTable<BranchId, ConditionalWeakTable<ProjectId, MetadataOnlyReferenceSet>>.CreateValueCallback s_createReferenceSetMap =
_ => new ConditionalWeakTable<ProjectId, MetadataOnlyReferenceSet>();
private static readonly ConditionalWeakTable<SolutionState, ConditionalWeakTable<ProjectId, MetadataOnlyReferenceSet>>.CreateValueCallback s_createReferenceSetMap = _ => new();
Copy link
Member Author

Choose a reason for hiding this comment

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

previous logic attempted to use BranchId here for reasons i honestly can't understand. Havin this be keyed by solution woudl be broken as we wouldn't be able to cache this across simple solution changes. However, we would like to be able to cache this for a particular lifetime of a solution as long as possible. Note: critically, this is not jsut an arbitrary "i will cache a projectid to a metadatarefset forever". instead, that set holds onto things like the 'dependent semantic version' of the project in that solution. So the references are only valid as long as no top level semantic information changed.


internal static MetadataReference GetOrBuildReference(
SolutionState solution,
Expand Down Expand Up @@ -64,7 +61,7 @@ internal static MetadataReference GetOrBuildReference(
// okay, proceed with whatever image we have

// now, remove existing set
var mapFromBranch = s_cache.GetValue(solution.BranchId, s_createReferenceSetMap);
var mapFromBranch = s_cache.GetValue(solution, s_createReferenceSetMap);
mapFromBranch.Remove(projectReference.ProjectId);

// create new one
Expand Down Expand Up @@ -106,31 +103,22 @@ internal static bool TryGetReference(
// okay, now use version based cache that can live multiple compilation as long as there is no semantic changes.

// get one for the branch
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
if (TryGetReferenceFromBranch(solution.BranchId, projectReference, finalOrDeclarationCompilation, version, out reference))
if (TryGetReferenceFromBranch(solution, projectReference, finalOrDeclarationCompilation, version, out reference))
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me if this will ever trigger now if the first map of compilation -> reference didn't have one, then I don't think we'd ever end up with a case where this other map has one?

Copy link
Member

Choose a reason for hiding this comment

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

If we still need this map, should it instead be keyed on ProjectId or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked with Jason offline. This entire type was rejiggered. It's now instance state on compilatino-tracker. Allowing it to both share data across forks, but also not have cahnges to branched solutions unintentionally leak backward to affect the main solution.

{
solution.Workspace.LogTestMessage($"Found already cached metadata for the branch and version {version}");
return true;
}

// see whether we can use primary branch one
var primaryBranchId = solution.Workspace.PrimaryBranchId;
if (solution.BranchId != primaryBranchId &&
TryGetReferenceFromBranch(primaryBranchId, projectReference, finalOrDeclarationCompilation, version, out reference))
{
solution.Workspace.LogTestMessage($"Found already cached metadata for the primary branch and version {version}");
return true;
}

// noop, we don't have any
reference = null;
return false;
}

private static bool TryGetReferenceFromBranch(
BranchId branchId, ProjectReference projectReference, Compilation finalOrDeclarationCompilation, VersionStamp version, out MetadataReference reference)
SolutionState state, ProjectReference projectReference, Compilation finalOrDeclarationCompilation, VersionStamp version, out MetadataReference reference)
{
// get map for the branch
var mapFromBranch = s_cache.GetValue(branchId, s_createReferenceSetMap);
var mapFromBranch = s_cache.GetValue(state, s_createReferenceSetMap);
// if we have one, return it
if (mapFromBranch.TryGetValue(projectReference.ProjectId, out var referenceSet) &&
(version == VersionStamp.Default || referenceSet.Version == version))
Expand Down
4 changes: 1 addition & 3 deletions src/Workspaces/Core/Portable/Workspace/Solution/Solution.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ private Solution(SolutionState state)
}

internal Solution(Workspace workspace, SolutionInfo.SolutionAttributes solutionAttributes, SerializableOptionSet options, IReadOnlyList<AnalyzerReference> analyzerReferences)
: this(new SolutionState(workspace.PrimaryBranchId, new SolutionServices(workspace), solutionAttributes, options, analyzerReferences))
: this(new SolutionState(new SolutionServices(workspace), solutionAttributes, options, analyzerReferences))
{
}

Expand All @@ -48,8 +48,6 @@ internal Solution(Workspace workspace, SolutionInfo.SolutionAttributes solutionA

internal SolutionServices Services => _state.Services;

internal BranchId BranchId => _state.BranchId;

internal ProjectState? GetProjectState(ProjectId projectId) => _state.GetProjectState(projectId);

/// <summary>
Expand Down
Loading