Fixed .NET SDK style project slowness in the editor

[TIHan]

This was something; took quite a bit of time tracking down, but now it's starting to make sense.

Edited

We changed the approach on how we fix this problem. Essentially, we are removing calls to UpdateProjectInfo on document opened/changed.

Test Plan

• New .NET SDK style project, measure time to first semantic highlighting
• Edit document in .NET SDK style project, time to semantic highlighting and autocomplete
• Generate large .NET SDK style solutions under tests/projects/stress and check load time and first semantic highlighting
• Check script in .NET SDK style project with Include=None.
• Check script in legacy project with Include=None
• Check script with same cases as above, but with Include=Compile
• Check script in no project without any project loaded
• Check out of project .fs file without a project loaded
• Check out of project .fs file where a project loaded .net sdk / legacy
• Check out of project .fsx script file where a project loaded .net sdk / legacy
• Close and re-open SortedMap.fs in Spreads, time to semantic highlighting after re-open.
• Close and re-open huge generated .NET SDK style .fs file, time to semantic highlighting after re-open.
• Add file in .NET SDK style project, check semantic highlighting
• Remove file in .NET SDK style project, check semantic highlighting

[KevinRansom]

Nice work.

[wallymathieu]

Nice 👍

[vasily-kirichenko]

This code deserves a small unit test :)

[forki]

But first merge it so that it's going into 15.7.2 ;-)

[davkean]

@Pilchie @cartermp This should be a servicing candidate.

[dsyme]

Great work.

[dsyme]

Could you take a while to think how to put this under automated test? It's important never to regress this...

The test suite has oodles of tests to check we dont reconpute/invalidate, counting number of parses, checks etc. But that only works if you get the project options right.

[dsyme]

The fix needs adjusting... We should not be calling UpdateProjectInfo at all during editing....

[TIHan]

I agree we should put a test here so we don't regress. At the top of my head, I'm not sure how to accomplish this as it's really in the middle of a lot of stuff. Unless we already have mocks, this will be quite a bit of work to do.

At some point we are going to refactor parts of the integration with the workspace. Perhaps during that time we can design something that is testable, isolated from the language service and workspace.

[TIHan]

I'll try to think of something.

[TIHan]

As for UpdateProjectInfo, @KevinRansom will have more context on why it's done the way it's done.

[dsyme]

As for UpdateProjectInfo, @KevinRansom will have more context on why it's done the way it's done.

Do you have a callstack for when UpdateProjectInfo is being called during standard editing?

[TIHan]

I don't on hand. I can get one next time I go into the office.

[TIHan]

Your best bet is to look at the workspace events and see how it eventually calls UpdateProjectInfo.

[dsyme]

Oh I see, yes. The following CPS-specific path from OnDocumentChanged to UpdateProjectInfo is just plain wrong - we should never have been updating the project info on every change to the document.

    member private this.OnDocumentChanged(projectId:ProjectId, documentId:DocumentId) = projectInfoManager.UpdateDocumentInfoWithProjectId(projectId, documentId, "OnDocumentChanged", invalidateConfig=false)

--> 

    member this.UpdateDocumentInfoWithProjectId(projectId:ProjectId, documentId:DocumentId, userOpName, invalidateConfig) =
        if workspace.IsDocumentOpen(documentId) then
            this.UpdateProjectInfoWithProjectId(projectId, userOpName, invalidateConfig)

-->

    member this.UpdateProjectInfoWithProjectId(projectId:ProjectId, userOpName, invalidateConfig) =
        ...
        | h when (h.IsCapabilityMatch("CPS")) ->
            ...
                    this.UpdateProjectInfo(tryGetOrCreateProjectId, projectId, projectSite, userOpName, invalidateConfig)

I asked this question when reviewing this code, when @cartermp proposed a fix for another manifestation of this problem in #4121:

@KevinRansom Do you know why we are doing anything at all for DocumentChanged? Was there a particular case that motivated this? e.g. why do we need to call UpdateDocumentInfoWithProjectId at all for OnDocumentChanged?

However I got no reply - @KevinRansom please make sure to keep track of questions like this and make sure they get a reply?

Anyway, I strongly believe that we that we should simply not be updating the project info on DocumentChanged. The only case where we might do that is for scripts, but they are handled differently.

[dsyme]

Anyway, I believe the correct fix is just to delete the event handler for OnDocumentChanged. And if that isn't a correct fix (I think it is) then we really need to work out why the event handler is needed....

[TIHan]

Deleting it would be ideal. We should also delete it from the textView.GetBuffer as well if we can. Opening a document shouldn't do that either, legacy doesn't.

[vasily-kirichenko]

Anyway, I strongly believe that we that we should simply not be updating the project info on DocumentChanged.

Can it result with loss of updating project options for FSX files when #r directives added/deleted?

[dsyme]

@vasily-kirichenko

Can it result with loss of updating project options for FSX files when #r directives added/deleted?

Good question. Script files are covered by this code in TryGetOptionsForDocumentOrProject:

            // The options for a single-file script project are re-requested each time the file is analyzed.  This is because the
            // single-file project may contain #load and #r references which are changing as the user edits, and we may need to re-analyze
            // to determine the latest settings.  FCS keeps a cache to help ensure these are up-to-date.
            match singleFileProjectTable.TryGetValue(projectId) with
            | true, (loadTime, _, _) ->
                ...

But the behaviour you mention should be checked as part of validating the fix .

We should also delete it from the textView.GetBuffer as well if we can.

@TIHan do you have a link for that?

[TIHan]

@vasily-kirichenko that's an interesting point. I wonder if adding #r's with the current fix will work or not.

For script files, we should be using the MiscellaneousFilesWorkspace which we are currently not. If we used that workspace, we could at least separate its specific behavior from the workspace for projects.

[TIHan]

@dsyme https://github.com/TIHan/visualfsharp/blob/a6fc8525b8fec6affdef69361d3847829b9d4314/vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs#L658 There it is. You can see the specific CPS path.

[TIHan]

It's not super clear but:
This is the legacy path: https://github.com/TIHan/visualfsharp/blob/a6fc8525b8fec6affdef69361d3847829b9d4314/vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs#L669

And this is the CPS path:
https://github.com/TIHan/visualfsharp/blob/a6fc8525b8fec6affdef69361d3847829b9d4314/vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs#L671

[dsyme]

For script files, we should be using the MiscellaneousFilesWorkspace which we are currently not. If we used that workspace, we could at least separate its specific behavior from the workspace for projects.

Yeah, there are a couple of glitches with script files (e.g. loss of colorization on save-as file rename) that can likely be fixed by updating script file options on changes like these

            | WorkspaceChangeKind.AdditionalDocumentAdded
            | WorkspaceChangeKind.AdditionalDocumentReloaded
            | WorkspaceChangeKind.AdditionalDocumentRemoved

Perhaps SetupNewTextView is just the wrong place to be setting up the options for out-of-project files and scripts, and that doing it in response to the events above will be more complete and accurate (and more portable too - we'd like to eventually use FSharp.Editor in VS for Mac, so the less VS-specific code the better. Roslyn and Roslyn CPS-specific code is ok).

[TIHan]

Yeah, there are a couple of glitches with script files (e.g. loss of colorization on save-as file rename) that can likely be fixed by updating script file options on changes like these, or by doing things differently in the first place

Then this fix needs to evolve a bit more. Sorry I didn't test that case.
Does the AdditionalDocument refer to files that are not part of the project, such as our script files maybe?

doing it in response to the events above will be more complete and accurate

This is exactly what we need to do. We should always listen to the workspace to give us what we need. Right now we rely on HandleCommandLineArgs method until we figure out how to get rid of it. We need to integrate with the evaluation/design-time builds.

[dsyme]

We should also delete it from the textView.GetBuffer as well if we can.

OK, I understand now. I added some comments to clarify the existing code here. Could you check the comments, particularly this one?

Let me explain a bit about useUniqueStamp. This stems from the time when ProjectSitesAndFiles.fs was shared between FSharp.Editor and FSharp.LanguageService. For FSharp.Editor this is always true, (and for the legacy FSharp.LanguageService this was always false). So the meaning of the flag is "are we using the unique stamping facility at all". Since we no longer share this file we can remove this flag, assuming it always to be true. It also means that changing the flag isn't quite the right fix.

[dsyme]

Yeah, there are a couple of glitches with script files (e.g. loss of colorization on save-as file rename) that can likely be fixed by updating script file options on changes like these, or by doing things differently in the first place

Then this fix needs to evolve a bit more. Sorry I didn't test that case.

Oh, they are existing glitches, not glitches in this PR. See #1944

Does the AdditionalDocument refer to files that are not part of the project, such as our script files maybe?

I believe so, but should be checked.

[dsyme]

@TIHan I removed the useUniqueStamp flag as part of #4891, to clarify.

[saul]

SetupNewTextView absolutely is the wrong place to setup for script files. I had a PR about a year ago which got rid of this override entirely. But when CPS support got added, it had extra stuff added to it. See https://github.com/Microsoft/visualfsharp/pull/2909/files

Instead of hooking onto SetupNewTextView, I hooked onto IVsRunningDocumentTable. The code is basically a stripped down version of the MiscellaneousFilesWorkspace (which we couldn’t use from F# at the time, we now can).

We should integrate with the MiscellaneousFilesWorkspace instead - it was designed entirely for script files.

[TIHan]

PR is updated. I'm going to add a generated netcore solution so we can easily test this.

[TIHan]

Tests have been added.

[cartermp]

@davkean @Pilchie I added internal bug 615543 so that we can bring it to shiproom for 15.7.3 or later.

[TIHan]

test Windows_NT Release_ci_part2 Build please

[dsyme]

Great job, love the tests. Now we all have really big .NET SDk solutions we can stress test on :)

[forki]

So the solution was to delete code that was already flag with weird comment?

[forki]

@cartermp it's too late for 15.7.2?

[cartermp]

@forki It's highly likely that it's too late for 15.7.2. Approvals for bugs we wanted to take for that went out last week.

[cartermp]

Another valueable one to user test is SQLProvider: https://github.com/fsprojects/SQLProvider

It's definitely suffering today

[dsyme]

@cartermp Are nightlies and master now usable with dev 15.7? We should put out some kind of announcement when that switch happens? (I'm not on dev 15.7 yet)

[cartermp]

Yes for both: https://dotnet.myget.org/feed/fsharp/package/vsix/VisualFSharp

We're producing 15.7.x versions for master and a build of a VSIX ought to do the same.

No real need for announcement, as this is the standard state of things. There was just an issue for a few days where we didn't merge back into master such that it would produce the correct VSIX versions.

[TIHan]

Doing some more testing today. So the MruCache for incremental builder is still getting invalidated, but this time after we get completions/intellisense. Something else is causing this, I'm going to investigate this.

[KevinRansom]

@TIHan, @dsyme, I don't recall why OnDocumentChanged causes invalidation. Removing it looks like the right thing to do.

[TIHan]

Ok, I think I know what's happening from my last comment.

It's because of multi-targetting. Mutli-targetted projects, are treated as two separate logical projects in the workspace. My hypothesis is that FCS only keys off of the project file name, rather than project file name and TFM. When this happens, there could be a race condition, and depending on the race condition will kick off invalidation and rebuild the world.

So this is a bug already today and is not from our current changes made here. Perhaps I can make another PR to fix multi-targeting.

[Pilchie]

Just a note about AdditionalFiles - they are not files that are outside of projects like scripts. Instead they are "extra" files added to to the compilation to be used as input to analyzers. Things like the "PublicAPI.unshipped.txt" in the roslyn repo.

As @saul said, we should definitely be hooking into MiscellaneousFilesWorkspace instead of doing this in SetupNewView.

[Pilchie]

Couple of questions:

1. If the test code is just generated boilerplate, why not generate it on each run instead of checking it in?
2. What's with the C# only solutions?
3. Can you point me to where this stuff is actually exercised, or is that still a manual process?

[Pilchie]

Strange that DocumentChanged was in this list twice already. Also - consider whether DocumentInfoChanged should result in a call.

[TIHan]

We should probably look into that. If we find that we need to add it in, we'll make a separate PR for that one.

[TIHan]

Thinking about this, once we move away from handlecommandlinargs, we will need to listen to DocumentInfoChanged most likely.

[dsyme]

We didn't see any DocumentInfoChanged events - when do they arise?

[Pilchie]

For example when the filename changes, etc:

        /// <summary>
        /// The document in the current solution had is info changed; name, folders, filepath
        /// </summary>
        DocumentInfoChanged = 17,

[TIHan]

We should probably not check in generated projects now that I'm thinking about it.

I think the C# solutions are there for us to test against the F# ones to see if we match perf.

All of this is a manual process. You just run fsi on the script.

[forki]

The advantage of checking it in is that contributors usually have it easier to use it. Also we have history of it (which can also be seen as a downside ;-)) Will Smith <notifications@github.com> schrieb am Mo., 14. Mai 2018, 18:24:

…

We should probably not check in generated projects now that I'm thinking about it. I think the C# solutions are there for us to test against the F# ones to see if we match perf. All of this is a manual process. You just run fsi on the script. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4889 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNHEUd-0K-9fJl5RrT__FQ6eokPU2ks5tya_LgaJpZM4T8PFR> .

[TIHan]

I'm removing the generated projects. I think it is fine to run the generation manually by invoking fsi. This keeps the commits much clearer than seeing 100s of generated files being committed. If we really want the generated projects to be committed, we can make a separate PR.

[dsyme]

I'm removing the generated projects. I think it is fine to run the generation manually by invoking fsi. This keeps the commits much clearer than seeing 100s of generated files being committed. If we really want the generated projects to be committed, we can make a separate PR.

Yes make a separate PR please. The huge advantage of having them in the tree is that everyone, everywhere has exactly the same baselines to test against for VS perf, without any effort. That's incredibly valuable.

We already have legacy projects checked in, so please, I really want .NET SDK ones too.

[forki]

As said above +1 for checking them in. We already saw benefits of checked in test projects in older issues.

[vasily-kirichenko]

I’m for checking them in too. Another reason: the generation script does not work on Mac out of the box due to the windows style slashes in paths.

[Pilchie]

Okay - I'll withdraw any objections about having the test files checked in.