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

Jump to bottom

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 all 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
108 changes: 37 additions & 71 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,83 +83,48 @@ 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 compileTimeSolution))
{
return cachedCompileTimeSolution;
}

using var _1 = ArrayBuilder<DocumentId>.GetInstance(out var configIdsToRemove);
using var _2 = ArrayBuilder<DocumentId>.GetInstance(out var documentIdsToRemove);
using var _1 = ArrayBuilder<DocumentId>.GetInstance(out var configIdsToRemove);
using var _2 = ArrayBuilder<DocumentId>.GetInstance(out var documentIdsToRemove);

foreach (var (_, projectState) in designTimeSolution.State.ProjectStates)
{
var anyConfigs = false;

foreach (var (_, configState) in projectState.AnalyzerConfigDocumentStates.States)
foreach (var (_, projectState) in designTimeSolution.State.ProjectStates)
{
if (IsRazorAnalyzerConfig(configState))
var anyConfigs = false;

foreach (var (_, configState) in projectState.AnalyzerConfigDocumentStates.States)
{
configIdsToRemove.Add(configState.Id);
anyConfigs = true;
if (IsRazorAnalyzerConfig(configState))
{
configIdsToRemove.Add(configState.Id);
anyConfigs = true;
}
}
}

// only remove design-time only documents when source-generated ones replace them
if (anyConfigs)
{
foreach (var (_, documentState) in projectState.DocumentStates.States)
// only remove design-time only documents when source-generated ones replace them
if (anyConfigs)
{
if (documentState.Attributes.DesignTimeOnly)
foreach (var (_, documentState) in projectState.DocumentStates.States)
{
documentIdsToRemove.Add(documentState.Id);
if (documentState.Attributes.DesignTimeOnly)
{
documentIdsToRemove.Add(documentState.Id);
}
}
}
}
}

var compileTimeSolution = designTimeSolution
.RemoveAnalyzerConfigDocuments(configIdsToRemove.ToImmutable())
.RemoveDocuments(documentIdsToRemove.ToImmutable());
compileTimeSolution = designTimeSolution
.RemoveAnalyzerConfigDocuments(configIdsToRemove.ToImmutable())
.RemoveDocuments(documentIdsToRemove.ToImmutable());

UpdateCachedCompileTimeSolution(designTimeSolution, compileTimeSolution);
_designTimeToCompileTimeSoution.Add(designTimeSolution, compileTimeSolution);
}

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.

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
42 changes: 3 additions & 39 deletions src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ namespace Microsoft.CodeAnalysis
/// </summary>
internal partial class SolutionState
{
// branch id for this solution
private readonly BranchId _branchId;

// the version of the workspace this solution is from
private readonly int _workspaceVersion;

Expand Down Expand Up @@ -73,7 +70,6 @@ internal partial class SolutionState
private readonly SourceGeneratedDocumentState? _frozenSourceGeneratedDocumentState;

private SolutionState(
BranchId branchId,
int workspaceVersion,
SolutionServices solutionServices,
SolutionInfo.SolutionAttributes solutionAttributes,
Expand All @@ -88,7 +84,6 @@ private SolutionState(
Lazy<HostDiagnosticAnalyzers>? lazyAnalyzers,
SourceGeneratedDocumentState? frozenSourceGeneratedDocument)
{
_branchId = branchId;
_workspaceVersion = workspaceVersion;
_solutionAttributes = solutionAttributes;
_solutionServices = solutionServices;
Expand All @@ -115,13 +110,11 @@ static Lazy<HostDiagnosticAnalyzers> CreateLazyHostDiagnosticAnalyzers(IReadOnly
}

public SolutionState(
BranchId primaryBranchId,
SolutionServices solutionServices,
SolutionInfo.SolutionAttributes solutionAttributes,
SerializableOptionSet options,
IReadOnlyList<AnalyzerReference> analyzerReferences)
: this(
primaryBranchId,
workspaceVersion: 0,
solutionServices,
solutionAttributes,
Expand All @@ -146,7 +139,7 @@ public SolutionState WithNewWorkspace(Workspace workspace, int workspaceVersion)

// Note: this will potentially have problems if the workspace services are different, as some services
// get locked-in by document states and project states when first constructed.
return CreatePrimarySolution(branchId: workspace.PrimaryBranchId, workspaceVersion: workspaceVersion, services: services);
return CreatePrimarySolution(workspaceVersion: workspaceVersion, services: services);
}

public HostDiagnosticAnalyzers Analyzers => _lazyAnalyzers.Value;
Expand All @@ -163,19 +156,6 @@ public SolutionState WithNewWorkspace(Workspace workspace, int workspaceVersion)

public SerializableOptionSet Options { get; }

/// <summary>
/// branch id of this solution
///
/// currently, it only supports one level of branching. there is a primary branch of a workspace and all other
/// branches that are branched from the primary branch.
///
/// one still can create multiple forked solutions from an already branched solution, but versions among those
/// can't be reliably used and compared.
///
/// version only has a meaning between primary solution and branched one or between solutions from same branch.
/// </summary>
public BranchId BranchId => _branchId;

/// <summary>
/// The Workspace this solution is associated with.
/// </summary>
Expand Down Expand Up @@ -235,8 +215,6 @@ private SolutionState Branch(
ProjectDependencyGraph? dependencyGraph = null,
Optional<SourceGeneratedDocumentState?> frozenSourceGeneratedDocument = default)
{
var branchId = GetBranchId();

if (idToProjectStateMap is not null)
{
Contract.ThrowIfNull(remoteSupportedProjectLanguages);
Expand All @@ -256,8 +234,7 @@ private SolutionState Branch(

var analyzerReferencesEqual = AnalyzerReferences.SequenceEqual(analyzerReferences);

if (branchId == _branchId &&
solutionAttributes == _solutionAttributes &&
if (solutionAttributes == _solutionAttributes &&
projectIds == ProjectIds &&
options == Options &&
analyzerReferencesEqual &&
Expand All @@ -271,7 +248,6 @@ private SolutionState Branch(
}

return new SolutionState(
branchId,
_workspaceVersion,
_solutionServices,
solutionAttributes,
Expand All @@ -288,19 +264,16 @@ private SolutionState Branch(
}

private SolutionState CreatePrimarySolution(
BranchId branchId,
int workspaceVersion,
SolutionServices services)
{
if (branchId == _branchId &&
workspaceVersion == _workspaceVersion &&
if (workspaceVersion == _workspaceVersion &&
services == _solutionServices)
{
return this;
}

return new SolutionState(
branchId,
workspaceVersion,
services,
_solutionAttributes,
Expand All @@ -316,15 +289,6 @@ private SolutionState CreatePrimarySolution(
frozenSourceGeneratedDocument: null);
}

private BranchId GetBranchId()
{
// currently we only support one level branching.
// my reasonings are
// 1. it seems there is no-one who needs sub branches.
// 2. this lets us to branch without explicit branch API
return _branchId == Workspace.PrimaryBranchId ? BranchId.GetNextId() : _branchId;
}

/// <summary>
/// The version of the most recently modified project.
/// </summary>
Expand Down
Loading