Remove the BranchId concept from teh workspace #57132

CyrusNajmabadi · 2021-10-13T23:02:20Z

We have a hypothesis that this info really isn't needed anymore and just adds unwarranted implementation and conceptual complexity.

Attempting to A/B this has proved challenging, as these values are computed and stored during the creation of hte workspace, which leads to high complexity if the workspace then tries to depend on options. Similarly, there was high difficulty ensuring that we could get a feature-flag reported, but ensure that it's value never changed during VS runtime (as that could lead to extremely bizarre behavior where we partially used branchIds and partially did not).

Because of this, we're going to go ahead in 17.1 and just try to remove this with the hope that there is no negative fallout. If there is, we have lots of coverage time and we can restore this if necessary.

CyrusNajmabadi · 2021-10-13T23:02:45Z

src/Features/Core/Portable/Workspace/CompileTimeSolutionProvider.cs

+ private readonly ConditionalWeakTable<Solution, Solution> _designTimeToCompileTimeSoution = new();
+#else
+ private ConditionalWeakTable<Solution, Solution> _designTimeToCompileTimeSoution = new();
+#endif


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

CyrusNajmabadi · 2021-10-13T23:06:12Z

src/Features/Core/Portable/Workspace/CompileTimeSolutionProvider.cs

- /// 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;


@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.

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

CyrusNajmabadi · 2021-10-13T23:07:39Z

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex.cs

@@ -54,8 +54,6 @@ public static async Task PrecalculateAsync(Document document, CancellationToken

 using (Logger.LogBlock(FunctionId.SyntaxTreeIndex_Precalculate, cancellationToken))
 {
- Debug.Assert(document.IsFromPrimaryBranch());


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.

CyrusNajmabadi · 2021-10-13T23:12:18Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs


- 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();


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.

CyrusNajmabadi · 2021-10-13T23:15:02Z

src/Workspaces/Remote/Core/SolutionAssetStorage.cs

@@ -53,7 +53,7 @@ private async ValueTask<Scope> StoreAssetsAsync(Solution solution, ProjectId? pr
 var id = Interlocked.Increment(ref s_scopeId);
 var solutionInfo = new PinnedSolutionInfo(
 id,
- fromPrimaryBranch: solutionState.BranchId == solutionState.Workspace.PrimaryBranchId,
+ fromPrimaryBranch: solution == solution.Workspace.CurrentSolution,


@tmat we still have this concept, but we could potentially remove it as all. All that happens with this is that when we hydrate the solution on the other side, if this bit is passed along, we also cache the solution into a field for reuse.

We could consider just always doing this.

Is there any race concern here about CurrentSolution moving forward?

CyrusNajmabadi · 2021-10-13T23:16:41Z

src/Workspaces/Remote/Core/SolutionChecksumUpdater.cs

- if (solution.BranchId != _workspace.PrimaryBranchId)
- {
- return;
- }


note: this logic legit just didn't make sense. if we ever have a workspace whose 'CurrentSOlution' was not the 'Primary' solution then that workspace is in a super f'ed up situation. the whole idea (generally, though of course we didnt' enforce) of the branchId was to say "is this, or was this the CurrentSolution of a workspace, as opposed to a fork of that". So if we literally just grabbed the .CurrentSolution of a workspace, and then found it was a fork itself... then that's just messed up supremely.

CyrusNajmabadi · 2021-10-13T23:20:46Z

src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs

@@ -55,8 +54,7 @@ internal class DiagnosticComputer
 _analysisKind = analysisKind;
 _analyzerInfoCache = analyzerInfoCache;

- // We only track performance from primary branch. All forked branch we don't care such as preview.
- _performanceTracker = project.IsFromPrimaryBranch() ? project.Solution.Workspace.Services.GetService<IPerformanceTrackerService>() : null;
+ _performanceTracker = project.Solution.Workspace.Services.GetService<IPerformanceTrackerService>();


this part i wasn't totally sure abuot... however the original code seems suspect to me. If we're trying track the performance of analyzers... why would we care which context they were running in? If running them on a forked solution is expensive... then c'est la vie, we now track that.

Also, if we really only want this for a particular workspace... then we should only export this from teh workspace we care about. Having it be a default service for all workspaces, that then attempts to shut itself off for some just defeats the purpose of default services.

jasonmalinowski

There's a bit more that can be deleted; also I'm not entirely sure about the insanity around the skeleton reference generation. Not that I will pretend to understand that code generally, I suspect either we can delete more of it, or we want to use something other than SolutionState in the one map.

src/Workspaces/Remote/Core/SolutionChecksumUpdater.cs

src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs

src/Workspaces/Core/Portable/Workspace/Workspace.cs

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

jasonmalinowski · 2021-10-13T23:19:14Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

@@ -106,31 +103,22 @@ internal class MetadataOnlyReference
 // okay, now use version based cache that can live multiple compilation as long as there is no semantic changes.

 // get one for the branch
- if (TryGetReferenceFromBranch(solution.BranchId, projectReference, finalOrDeclarationCompilation, version, out reference))
+ if (TryGetReferenceFromBranch(solution, projectReference, finalOrDeclarationCompilation, version, out reference))


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?

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

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.

CyrusNajmabadi · 2021-10-14T00:32:08Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

@@ -14,28 +14,46 @@

 namespace Microsoft.CodeAnalysis
 {
- internal class MetadataOnlyReference
+ internal class CachedMetadataOnlyReferences


this type is very changed will doc heavily.

CyrusNajmabadi · 2021-10-14T00:43:29Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

+ /// semantics) will not leak backward to a prior compilation tracker point, causing it to see a view of the world
+ /// inapplicable to its current snapshot.
+ /// </summary>
+ internal class CachedSkeletonReferences


renamed the type to be clearer about what it stores.

File would need renaming as well.

CyrusNajmabadi · 2021-10-14T00:43:46Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

+ /// with a reference to this compilation (and the same <see cref="MetadataReferenceProperties"/> is allowed
+ /// to reference it using the same <see cref="MetadataReference"/>.
+ /// </summary>
+ private static readonly ConditionalWeakTable<Compilation, MetadataOnlyReferenceSet> s_compilationToReferenceMap = new();


commented above, but this should always be safe to use as a static CWT.

CyrusNajmabadi · 2021-10-14T00:45:27Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

+ /// <summary>
+ /// Mapping from <see cref="ProjectId"/> to the skeleton <see cref="MetadataReference"/>s for it.
+ /// </summary>
+ private ImmutableDictionary<ProjectId, MetadataOnlyReferenceSet> _projectIdToReferenceMap;


this now moved to be instance state so that as compilation trackers fork, later forks do not bleed back to prior forks. Note; because we just hand-off the ImmutableDictionary to the forks, it also means that forks don't see what prior trackers computed if they forked first, then the prior tracker computed this data.

My feeling is that that's ok. And it prevents us from keeping track of a long chain of prior CachedSkeletonReferences types.

So we ideally get the benfit, but without a high cost to that.

jasonmalinowski · 2021-10-14T01:10:28Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

+ /// <para/>
+ /// This approach works, but has the caveat that live cross language semantics are only possible when the 
+ /// skeleton assembly can be built. This should always be the case for correct code, but it may not be the
+ /// case for code with errors depending on if the respective language compilat is unable to generate the skeleton


Suggested change

/// case for code with errors depending on if the respective language compilat is unable to generate the skeleton

/// case for code with errors depending on if the respective language compiler is unable to generate the skeleton

jasonmalinowski · 2021-10-14T01:22:28Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

+
+ lock (_gate)
+ {
+ _projectIdToReferenceMap = _projectIdToReferenceMap.Remove(projectReference.ProjectId);


Why the remove/add in this case?

jasonmalinowski · 2021-10-14T01:35:35Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs

@@ -44,22 +44,26 @@ private partial class CompilationTracker : ICompilationTracker
 // guarantees only one thread is building at a time
 private readonly SemaphoreSlim _buildLock = new(initialCount: 1);

+ public CachedSkeletonReferences CachedSkeletonReferences { get; }


I'm not quite understanding why this is being held by the CompilationTracker in this case, especially given that inside the CachedSkeletonReferences is also a map keyed by ProjectId. Considering a case where you have project A that's a C# project and it's consumed both by B and C, both VB. I would have assumed we want to be sharing the skeleton references that are being consumed by B and C so you'd only need one CachedSkeletonReferences for the entire solution. And if we fork either B or C, we can still keep reusing the same skeletons for A across both. I'd expect either:

There's just one for the entire solution, or.

A CompilationTracker's map is holding the skeletons created for that project, which doesn't need further keying.

Did you intend this to be held by the SolutionState instead?

jasonmalinowski · 2021-10-14T01:37:53Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

+ /// semantics) will not leak backward to a prior compilation tracker point, causing it to see a view of the world
+ /// inapplicable to its current snapshot.
+ /// </summary>
+ internal class CachedSkeletonReferences


File would need renaming as well.

jasonmalinowski · 2021-10-14T01:38:32Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

+ /// <summary>
+ /// Mapping from <see cref="ProjectId"/> to the skeleton <see cref="MetadataReference"/>s for it.
+ /// </summary>
+ private ImmutableDictionary<ProjectId, MetadataOnlyReferenceSet> _projectIdToReferenceMap;


Does anything ever ensure we remove things from this when projects are removed?

no. but this keys from project-id to weakref. so worse case we have a few extra entries in the dictionary.

tmat · 2021-10-14T01:52:10Z

src/Workspaces/Core/Portable/Workspace/Solution/MetadataOnlyReference.cs

+ /// semantics) will not leak backward to a prior compilation tracker point, causing it to see a view of the world
+ /// inapplicable to its current snapshot.
+ /// </summary>
+ internal class CachedSkeletonReferences


class

sealed

NTaylorMullen · 2021-10-14T02:16:35Z

src/Features/Core/Portable/Workspace/CompileTimeSolutionProvider.cs

+#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


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?

CyrusNajmabadi · 2021-10-22T20:41:38Z

@jasonmalinowski plz review.

jasonmalinowski

Looks fine; some possible cleanup is still available in CompileTimeSolutionProvider.cs I think. The check in SolutionAssetStorage.cs is also something I can't really speak to.

jasonmalinowski · 2021-10-22T20:45:12Z

src/Workspaces/Remote/Core/SolutionAssetStorage.cs

@@ -53,7 +53,7 @@ private async ValueTask<Scope> StoreAssetsAsync(Solution solution, ProjectId? pr
 var id = Interlocked.Increment(ref s_scopeId);
 var solutionInfo = new PinnedSolutionInfo(
 id,
- fromPrimaryBranch: solutionState.BranchId == solutionState.Workspace.PrimaryBranchId,
+ fromPrimaryBranch: solution == solution.Workspace.CurrentSolution,


Is there any race concern here about CurrentSolution moving forward?

src/Features/Core/Portable/Workspace/CompileTimeSolutionProvider.cs

jasonmalinowski · 2021-10-22T21:10:16Z

src/Features/Core/Portable/Workspace/CompileTimeSolutionProvider.cs

@@ -123,42 +122,17 @@ public Solution GetCompileTimeSolution(Solution designTimeSolution)
 .RemoveAnalyzerConfigDocuments(configIdsToRemove.ToImmutable())
 .RemoveDocuments(documentIdsToRemove.ToImmutable());

- UpdateCachedCompileTimeSolution(designTimeSolution, compileTimeSolution);
+#if NETCOREAPP
+ _designTimeToCompileTimeSoution.AddOrUpdate(designTimeSolution, compileTimeSolution);


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

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

This reverts commit 83a728a, reversing changes made to a4ec465.

Revert "Merge pull request #57132 from CyrusNajmabadi/branchId3"

…merges/release/dev17.1-vs-deps-to-main * upstream/release/dev17.1-vs-deps: Cleanup missed suggestion Update src/Workspaces/Core/Portable/Classification/ClassifierHelper.cs SpillSequenceSpiller.Spill - ensure sequences under ref assignments are spilled. (#58657) (#58788) Correctly update the list of generators for a call to WithAnalyzerReferences Rename parameter for what it's actually doing Handle AdditiveSpans in either order Reduce Stack Trace Explorer automatic open behavior scope (#58819) Revert 58892 (#58930) Mark APIs as shipped (#58776) Revert "Merge pull request #57132 from CyrusNajmabadi/branchId3" Fix race in WorkCoordinator when handling document reanalysis Remove unnecessary lock access setting UIContexts in the workspace

CyrusNajmabadi added 2 commits October 13, 2021 15:22

Remove branch-ids from teh workspace

0199590

Flow downwards

2715c69

CyrusNajmabadi requested review from tmat, jasonmalinowski, NTaylorMullen and dibarbet October 13, 2021 23:02

CyrusNajmabadi requested a review from a team as a code owner October 13, 2021 23:02

dotnet-issue-labeler bot added the Area-IDE label Oct 13, 2021

CyrusNajmabadi commented Oct 13, 2021

View reviewed changes

inline

d6001fc

CyrusNajmabadi commented Oct 13, 2021

View reviewed changes

revert

6812155

CyrusNajmabadi commented Oct 13, 2021

View reviewed changes

jasonmalinowski reviewed Oct 13, 2021

View reviewed changes

CyrusNajmabadi added 3 commits October 13, 2021 16:31

Delete validaiton

d1f781c

Simplify

9aae5f7

Fixup

7782d4b

CyrusNajmabadi commented Oct 14, 2021

View reviewed changes

CyrusNajmabadi force-pushed the branchId3 branch from 96da5be to 90ffb2f Compare October 14, 2021 00:40

moved cached metadata refs to instance state

effa0bf

CyrusNajmabadi force-pushed the branchId3 branch from 90ffb2f to effa0bf Compare October 14, 2021 00:43

CyrusNajmabadi commented Oct 14, 2021

View reviewed changes

CyrusNajmabadi requested a review from jasonmalinowski October 14, 2021 00:51

jasonmalinowski reviewed Oct 14, 2021

View reviewed changes

tmat reviewed Oct 14, 2021

View reviewed changes

NTaylorMullen reviewed Oct 14, 2021

View reviewed changes

CyrusNajmabadi added 5 commits October 14, 2021 12:10

Temp

d83c686

Temp

fea7fc9

Move skeleton refs to ProjectState

ff2362b

Merge branch 'main' into branchId3

7155386

revert'

19748d1

CyrusNajmabadi requested review from jasonmalinowski and NTaylorMullen October 22, 2021 20:26

jasonmalinowski approved these changes Oct 22, 2021

View reviewed changes

CyrusNajmabadi added 3 commits October 23, 2021 08:45

Merge remote-tracking branch 'upstream/main' into branchId3

57c7932

Add comment

e3f9cbc

Simplify since we're under a lock

48c9652

CyrusNajmabadi enabled auto-merge October 25, 2021 20:31

CyrusNajmabadi merged commit 83a728a into dotnet:main Oct 25, 2021

ghost added this to the Next milestone Oct 25, 2021

RikkiGibson modified the milestones: Next, 17.1.P1 Oct 25, 2021

CyrusNajmabadi deleted the branchId3 branch October 26, 2021 00:00

CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Jan 12, 2022

Revert "Merge pull request dotnet#57132 from CyrusNajmabadi/branchId3"

c04a5e8

This reverts commit 83a728a, reversing changes made to a4ec465.

CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Jan 13, 2022

Revert "Merge pull request dotnet#57132 from CyrusNajmabadi/branchId3"

3fd6248

This reverts commit 83a728a, reversing changes made to a4ec465.

CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Jan 14, 2022

Revert "Merge pull request dotnet#57132 from CyrusNajmabadi/branchId3"

a6ecada

This reverts commit 83a728a, reversing changes made to a4ec465.

CyrusNajmabadi added a commit that referenced this pull request Jan 20, 2022

Merge pull request #58856 from CyrusNajmabadi/revertBranchIdBackport

30573e1

Revert "Merge pull request #57132 from CyrusNajmabadi/branchId3"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the BranchId concept from teh workspace #57132

Remove the BranchId concept from teh workspace #57132

CyrusNajmabadi commented Oct 13, 2021

CyrusNajmabadi Oct 13, 2021

CyrusNajmabadi Oct 13, 2021

NTaylorMullen Oct 14, 2021

CyrusNajmabadi Oct 13, 2021

CyrusNajmabadi Oct 13, 2021

CyrusNajmabadi Oct 13, 2021

jasonmalinowski Oct 22, 2021

CyrusNajmabadi Oct 13, 2021

CyrusNajmabadi Oct 13, 2021

jasonmalinowski left a comment

jasonmalinowski Oct 13, 2021

jasonmalinowski Oct 13, 2021

CyrusNajmabadi Oct 14, 2021

CyrusNajmabadi Oct 14, 2021

CyrusNajmabadi Oct 14, 2021

jasonmalinowski Oct 14, 2021

CyrusNajmabadi Oct 14, 2021

CyrusNajmabadi Oct 14, 2021

jasonmalinowski Oct 14, 2021

jasonmalinowski Oct 14, 2021

jasonmalinowski Oct 14, 2021

CyrusNajmabadi Oct 14, 2021

jasonmalinowski Oct 14, 2021

jasonmalinowski Oct 14, 2021

CyrusNajmabadi Oct 14, 2021

tmat Oct 14, 2021

NTaylorMullen Oct 14, 2021

CyrusNajmabadi commented Oct 22, 2021

jasonmalinowski left a comment

jasonmalinowski Oct 22, 2021

jasonmalinowski Oct 22, 2021

CyrusNajmabadi Oct 25, 2021

	/// case for code with errors depending on if the respective language compilat is unable to generate the skeleton
	/// case for code with errors depending on if the respective language compiler is unable to generate the skeleton

Remove the BranchId concept from teh workspace #57132

Remove the BranchId concept from teh workspace #57132

Conversation

CyrusNajmabadi commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 22, 2021

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment