-
Notifications
You must be signed in to change notification settings - Fork 790
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
Fix solution reload and config change issues #3025
Conversation
@@ -13,7 +13,7 @@ type internal ValueStrength<'T when 'T : not struct> = | |||
| Weak of WeakReference<'T> | |||
#endif | |||
|
|||
type internal AgedLookup<'Token, 'Key, 'Value when 'Value : not struct>(keepStrongly:int, areSame, ?requiredToKeep, ?onStrongDiscard, ?keepMax: int) = | |||
type internal AgedLookup<'Token, 'Key, 'Value when 'Value : not struct>(keepStrongly:int, areSimilar, ?requiredToKeep, ?onStrongDiscard, ?keepMax: int) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just renaming arguments for consistency
src/fsharp/vs/service.fs
Outdated
@@ -2589,31 +2589,37 @@ type BackgroundCompiler(referenceResolver, projectCacheSize, keepAssemblyContent | |||
}) | |||
|
|||
member bc.InvalidateConfiguration(options : FSharpProjectOptions) = | |||
// This operation can't currently be cancelled nor awaited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalidate configuration is adjusted to explicitly invalidate any similar background build, i.e. one with same project file. THis is used when switching configurations
@@ -401,7 +410,7 @@ type internal FSharpChecker = | |||
/// can be used to marginally increase accuracy of intellisense results in some situations. | |||
/// </param> | |||
/// | |||
member CheckFileInProjectIfReady : parsed: FSharpParseFileResults * filename: string * fileversion: int * source: string * options: FSharpProjectOptions * ?textSnapshotInfo: obj -> Async<FSharpCheckFileAnswer option> | |||
member CheckFileInProjectAllowingStaleCachedResults : parsed: FSharpParseFileResults * filename: string * fileversion: int * source: string * options: FSharpProjectOptions * ?textSnapshotInfo: obj -> Async<FSharpCheckFileAnswer option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renaming
@@ -64,6 +64,8 @@ type internal FSharpCheckerProvider | |||
|
|||
member this.Checker = checker.Value | |||
|
|||
type Refreshable<'T> = 'T * (unit -> 'T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is that we have a function to recalc the options when we need to, But we also can deliver the latest copy of the options to other callers
@@ -267,7 +268,7 @@ module internal Tokenizer = | |||
data.[i] <- None | |||
i <- i + 1 | |||
|
|||
let private dataCache = ConditionalWeakTable<DocumentId, SourceTextData>() | |||
let private dataCache = ConditionalWeakTable<DocumentId, ConcurrentDictionary<string list, SourceTextData>>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fix to colorization - when switching configurations, colorization information was not updated, e.g. #if DEBUG
sections.
And this And this Again and again |
This is ready (once green) |
@vasily-kirichenko If you could give it another go that would be great. |
@vasily-kirichenko @KevinRansom @brettfo @saul and others - could I request a code review on this please? Thanks |
@@ -44,21 +44,30 @@ namespace Internal.Utilities.Collections | |||
new : keepStrongly:int | |||
* areSame:('Key * 'Key -> bool) | |||
* ?isStillValid:('Key * 'Value -> bool) | |||
* ?areSameForSubsumption:('Key * 'Key -> bool) | |||
* ?areSimilar:('Key * 'Key -> bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A XML Doc comment would be great here explaining what "similarity" means here and what it will be used for.
projectTable.TryRemove(projectId) |> ignore | ||
member this.RefreshInfoForProjectsThatReferenceThisProject(projectId: ProjectId) = | ||
// Search the projectTable for things to refresh | ||
for KeyValue(otherProjectId, ((referencedProjectIds, _options), refresh)) in projectTable.ToArray() do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious, why ToArray
here?
Same endless assertions. |
I can confirm the 3 issues fixed. |
src/fsharp/CompileOps.fs
Outdated
fslibRoot (* , sprintf "v%d.%d" v1 v2 *) | ||
with e -> | ||
error(Error(FSComp.SR.buildErrorOpeningBinaryFile(filename, e.Message), rangeStartup)) | ||
if (* validate && *) fslibReference.ProjectReference.IsNone then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why validate commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the commented out text now. It turns out this must be being called by fsc.exe
with validate=false
, so the error message sequence was being changed by checking this, but that is a separate issue
@@ -64,6 +65,8 @@ type internal FSharpCheckerProvider | |||
|
|||
member this.Checker = checker.Value | |||
|
|||
type Refreshable<'T> = 'T * (bool -> 'T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XML doc comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
let ok,originalOptions = optionsAssociation.TryGetValue(projectContext) | ||
let updatedOptions = site.CompilerFlags() | ||
if not ok || originalOptions <> updatedOptions then | ||
let updatedFiles = site.SourceFilesOnDisk() |> wellFormedFilePathSetIgnoreCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shadowing the exact same above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good catch, fixed
|> Seq.toArray | ||
static let rec referencedProjectsOf (tryGetOptionsForReferencedProject, projectSite:IProjectSite, extraProjectInfo, serviceProvider:System.IServiceProvider, useUniqueStamp) = | ||
[| for (p,ps) in referencedProvideProjectSites (projectSite, serviceProvider) do | ||
match fullOutputAssemblyPath p with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the indentation off by one here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
// We wait until allproject configurations have been updated before updating our MSBuild state | ||
projNode.UpdateMSBuildState() | ||
projNode.SetProjectFileDirty(projNode.IsProjectFileDirty) | ||
projNode.ComputeSourcesAndFlags() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is UpdateProjectCfg_Done called when OnAfterActiveSolutionCfgChange and OnActiveProjectCfgChangeBatchEnd isn't? Are we definitely only calling ComputeSourcesAndFlags once per project on configuration change? Also is the wait dialog still open when we call ComputeSourcesAndFlags here (is this method called before OnAfterActiveSolutionCfgChange)?
I'm worried we've just tripled configuration change/solution load time by calling ComputeSourcesAndFlags so many times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I see for the set of events triggered when loading tests\projects\misc\TestProjectChanges.sln
. Looking at this,
-
I believe you are right that we are doing ComputeSourcesAndFlags twice per project on solution load. But suspect we were doing that before, and it is because we are getting two complete sets of
OnActiveProjectCfgChangeBatchBegin
/OnActiveProjectCfgChangeBatchEnd
events sent through. -
The cost of
ComputeSourcesAndFlags
has been much reduced since use graph of option objects - fix 2793 #3002 since it uses the stored computed options for referenced projects -
It is important not to do any ComputeSourcesAndFlags until we have updated the configuration on all the projects. So I think we have to do this in OnActiveProjectCfgChangeBatchEnd.
Loading tests\projects\misc\TestProjectChanges.sln
- OnActiveProjectCfgChangeBatchBegin x 3
- IVsUpdateSolutionEvents.OnActiveProjectCfgChange(null) x 3
- IVsUpdateSolutionEvents.OnActiveProjectCfgChange(not-null) x 6
- OnActiveProjectCfgChangeBatchEnd x 3
Then we get a "duplicate" set of Batch events
- OnActiveProjectCfgChangeBatchBegin x 3
- OnActiveProjectCfgChangeBatchEnd x 3
Then something else forces ComputeSourcesAndFlags on each project
Switching to "Release"
- OnBeforeActiveSolutionCfgChange x 3
- OnActiveProjectCfgChangeBatchBegin x 3
- IVsUpdateSolutionEvents.OnActiveProjectCfgChange(null) x 3
- IVsUpdateSolutionEvents.OnActiveProjectCfgChange(not-null) x 6
- OnActiveProjectCfgChangeBatchEnd x 3
- OnAfterActiveSolutionCfgChange x 3 ComputeSourcesAndFlags
On prompted solution reload after a project file has been edited
- OnActiveProjectCfgChangeBatchBegin x 3
- IVsUpdateSolutionEvents.OnActiveProjectCfgChange(null) x 3
- IVsUpdateSolutionEvents.OnActiveProjectCfgChange(not-null) x 6
- OnActiveProjectCfgChangeBatchEnd x 3
Then we get a "duplicate" set of Batch events
- OnActiveProjectCfgChangeBatchBegin x 3
- OnActiveProjectCfgChangeBatchEnd x 3
Then something else forces ComputeSourcesAndFlags on each project
On individual project reload
- OnActiveProjectCfgChange
Also I never see these being called in the scenarios I've tested - if you know a sequence that triggers them please let me know :)
- IVsUpdateSolutionEvents2.OnActiveProjectCfgChange
- IVsUpdateSolutionEvents2.UpdateProjectCfg_Begin
- IVsUpdateSolutionEvents2.UpdateProjectCfg_Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just had a read through and I'm a bit concerned that we're now calling ComputeSourcesAndFlags 3 times when we change solution configuration and on solution load - that operational is painfully slow. Can you check that we're only calling it once in these scenarios?
@saul Thanks for the review! I'll go through the items now |
@Saull Could you check the last two commits? I've updated to only call After this change I don't see any value in the I must say this logic is a right mess. I can't wait to ditch the whole project system and replace it with the Roslyn one |
@saul OK, I think this is ready @vasily-kirichenko I believe the assertion is a different problem - we've certainly seen it before - I've been able to repro it sometimes but not consistently. |
I've not seen this before, I'm confident it's a separate problem but it would be good to address it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely convinced by the ComputeSourcesAndFlags changes in the solution configuration event code in Project.fs - it looks like we're calling ComputeSourcesAndFlags the same number of times now in the same scenarios. What's actually been fixed here?
The batchState
tracking ensured that for each batch, we only called ComputeSourcesAndFlags for each project once. Also out of interest why is UpdateMSBuildState necessary before we call ComputeSourcesAndFlags? It may be worth commenting the reason.
// By default, the F# project system keeps its own internal Configuration and Platform in sync with the current active | ||
// Configuration and Platform by listening for OnActiveProjectCfgChange events. However there is one case where the | ||
// active cfg changes without an event, and this is during 'Batch Build'. So we listen for the start and end of | ||
// Batch Build, and manually update the project to the active cfg before/after to set/reset the config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has this comment been removed? Is this no longer the case? Your other comment didn't lead me to believe that you tried doing a batch build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment below.
|
||
VSConstants.S_OK | ||
|
||
member x.OnAfterActiveSolutionCfgChange(_oldCfg, _newCfg) = | ||
|
||
// We have updated the MSBuild state of each project. Now we can call ComputeSourcesAndFlags | ||
projNode.ComputeSourcesAndFlags() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we doing this here? This means that the progress dialog is going to close way before all of the ComputeSourcesAndFlags are complete.
We should reference count how many times waitDialog was (going to be) opened, and then only dispose of it when it reaches 0.
member x.OnActiveProjectCfgChangeBatchEnd() = | ||
batchState <- NonBatch | ||
projNode.UpdateMSBuildState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any comments as to why this is now necessary? Why at this stage and not at OnActiveProjectCfgChange?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good questions. TBH I just fiddled around for about 3 days (yes really) until it seemed to work in every scenario I tried it in. Almost every other variation seemed to fail in some way or another.
I didn't however try a batch build. I had misunderstood what that actually was, sorry.
You are correct that the dialog goes away early though another "computing sources and flags for project" replaces it.
I'll see if I can find a less invasive set of changes. Perhaps there is just one fix needed in this code after all.
if GetCaption(pHierProj) = GetCaption(projNode.InteropSafeIVsHierarchy) then | ||
// This code matches what ProjectNode.SetConfiguration would do; that method cannot be called during a build, but at this | ||
// current moment in time, it is 'safe' to do this update. | ||
let _,currentConfigName = Utilities.TryGetActiveConfigurationAndPlatform(projNode.Site, projNode.ProjectIDGuid) | ||
MSBuildProject.SetGlobalProperty(projNode.BuildProject, ProjectFileConstants.Configuration, currentConfigName.ConfigName) | ||
MSBuildProject.SetGlobalProperty(projNode.BuildProject, ProjectFileConstants.Platform, currentConfigName.MSBuildPlatform) | ||
projNode.UpdateMSBuildState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
@KevinRansom This is not yet ready to go in (at least until @saul approves :)) |
A record of the events raised on various project actions: Loading tests\projects\misc\TestProjectChanges.sln
Then we get a "duplicate" set of Batch events
Then something else forces ComputeSourcesAndFlags on each project Switching to "Release"
On prompted solution reload after a project file has been edited
Then we get a "duplicate" set of Batch events
Then something else forces ComputeSourcesAndFlags on each project On individual project reload
On batch build:
I never see this being called in the scenarios I've tested
|
@saul Here are my further investigations
So in short we should have the same number of Manual testing: tests\proejcts\misc\TestProjectChanges\TestProjectChanges.sln
VisualFSharp.sln
I think this is the best I can do. Though I'll try again if you ask me :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for investigating this @dsyme, just looked over your latest changes and my mind is at ease 😄 This looks a lot better.
@saul The thanks is to you for reviewing so carefully. It's about the most important thing we can do. |
@KevinRansom this is ready to merge (unless you wish to add additional reviews) |
* fix config change * Fix configuration changes * Fix colorization cache for changing defines * fix config switching * add manual test cases * Fix test * minor nit * use correct reference count * cleanup decrement logic * fix build * Update CompileOps.fs * adjust unit tests mocks * adjust unit tests mocks * fix test * add new misc project test * do ComputeSourcesAndFlags later * do ComputeSourcesAndFlags once * variations * keep dialog open
This fixes a bunch of bad regressions around solution reload and "change configuration" actions
The problems were:
Testing:
I've added a project under tests\projects\misc for manually checking behaviour when switching configs or making other changes to a project
I've checked the following load
devenv tests\projects\stress\big\dense\Dense.sln
devenv tests\projects\stress\huge\dense\Dense.sln
devenv tests\projects\stress\big\shallow\Shallow.sln
Notes on testing:
@saul @vasily-kirichenko and others - I'd be grateful if you could try this out and validate that it fixes the bugs I've marked. Also check for loss of performance - we get a lot of "Project has updated" signals so have to recompute a lot of flags.