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.

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

add option to disable in-memory cross-project references #3328

Closed
wants to merge 2 commits into from

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Jul 13, 2017

No description provided.

@dsyme
Copy link
Contributor Author

dsyme commented Jul 13, 2017

I put the option on the “Intellisense” page for simplicity, we should probably have it on a new page of its own for “Language Service Performance”

image

@vasily-kirichenko
Copy link
Contributor

It would be great if you describe what this setting changes exactly, I mean what features degrade if it turned on.

@dsyme
Copy link
Contributor Author

dsyme commented Jul 13, 2017

@vasily-kirichenko Yes, will do. The change is part of an investigation for a problem with a particular user, it's not for immediate pull

@dsyme dsyme changed the title add option to disable in-memory cross-project references [WIP] add option to disable in-memory cross-project references Jul 13, 2017
@dsyme
Copy link
Contributor Author

dsyme commented Jul 13, 2017

@vasily-kirichenko Here are my notes from an internal email. Thanks for reminding me to be transparent

... a scenario of excessive memory usage in a large solution is described via private screenshots ... My response below, slightly edited


I believe there are several things potentially going on here:

  1. Using stale results.

    In VS2015 we allowed the use of “stale” typechecking results in intelliisense. In VS2017 we initially did not, but in this PR we switched to allow the use of stale results after 1 second timeout. I presume that PR is in the bits being used.

  2. Allowing in-memory cross-project references without on-disk DLLs.

    In VS2015 we required referenced projects to be compiled to DLLs and we read metadata from those DLLs

    In VS2017 we use in-memory cross-project references where we typecheck the code in memory, see this PR.

    This means that if you have a chain in projects A8 -> A7 -> … -> A1 (arrows represent direction of references) and you make a change in project A1 then go to project A8 then different things happen in VS2015 and VS2017:

    • In VS2015, you have to manually rebuild A1. Experienced Visual F# users are used to this and it is hardwired in their brain. After you have rebuilt A1.dll, then you then go to A8, the language service will immediately pick up the changes to A1.dll, without rebuilding A2…A7. And things will be pretty fast if A8 is small.

    • In VS2017, you don’t have to manually rebuild A1. However the LS will recheck all of A1…A7 before the changes will appear in A8.

In-memory cross-project references can also use significantly more memory. People who had the Visual F# Power Tools installed in VS2015 would experience overall less memory usage, because VFPT in VS2015 used a duplicate copy of the language service state with cross-project references enabled. However in the scenario described, the Visual F# Power Tools are not enabled.

  1. Long delays in language service availability when changes have happened to referenced projects.

    The changes in (2) above meant that, in some cases, requests submitted to the language service had large chunks of time where they were not cancellable (i.e. not checking IsCancellationRequested) – especially when dependent projects (A1 … A7) were being re-checked. This meant that the usage of stale results (see Add contributing guidelines to repo root #1) would not kick in, because the compiler service was busy processing a non-cancellable chunk of work.

    This problem was present in VS2015, but was made worse by enabling in-memory cross-project references.

    I believe this issue is entirely fixed by Cancel background work  #3063 but we have not yet pulled that.

So I believe (2) is the primary cause of the behaviour in the scenario. Essentially we are paying the cost for eliminating the manual builds of project references – i.e. doing things “right” but much more slowly in large solutions.

Tactically, I believe the two things would be

  1. Have the customer build and trial of VFT with cross-project references optionally disabled. Unfortunately we didn’t include an option to disable this feature in 15.3 – I kept meaning to do it since I was suspicious of it, but ran into problems understanding what was going on with the options page. To add the option, you would need to add a checkbox in the UI, propagate the flag and then change this:

     let referencedProjectFileNames, referencedProjectOptions = 
         referencedProjectsOf(tryGetOptionsForReferencedProject, projectSite, extraProjectInfo, serviceProvider, useUniqueStamp) 
         |> Array.unzip
    

To this:

    let referencedProjectFileNames, referencedProjectOptions = 
        if inMemoryCrossProjectReferencesEnabled then
            [| |], [| |]
        else
            referencedProjectsOf(tryGetOptionsForReferencedProject, projectSite, extraProjectInfo, serviceProvider, useUniqueStamp) 
            |> Array.unzip

If we verify that disabling this solves the problem, I would recommend that the inMemoryCrossProjectReferencesEnabled option be enabled by default, and that the documentation say something like this:

disabling this option may reduce memory usage and increase response time for language service features, but will require manual rebuilds of dependent projects before changes are propagated to the rest of the solution

I will prepare a PR which the customer can pull or we can pull into master

  1. Have the customer build and trial VFT with the Cancel background work PR pulled. Note this isn’t a thing that can easily be turned on/off, and step 1 should be performed independently to verify things. If we verify this helps improve LS availability then we should also just pull that PR into master so people using nightlies are testing it

Two other cards to play I can think of are:

  1. add an option to reduce the project cache size, which is currently set to 200 (in VS2015 it was only 3, because in-memory cross-project references were not enabled)

  2. add an option to enable the funky MaxMemory feature, which basically says “if the process memory reaches 2.3GB, flush all caches and reduce the project cache size to 1”. Or even enable it by default.

@dsyme dsyme changed the title [WIP] add option to disable in-memory cross-project references add option to disable in-memory cross-project references Jul 13, 2017
@dsyme
Copy link
Contributor Author

dsyme commented Jul 13, 2017

this is ready

@dsyme
Copy link
Contributor Author

dsyme commented Jul 13, 2017

@vasily-kirichenko I'm not sure if you're in the mood but you might be interested in these if they are helpful to you as a user:

add an option to reduce the project cache size, which is currently set to 200 (in VS2015 it was only 3, because in-memory cross-project references were not enabled)

add an option to enable the funky MaxMemory feature, which basically says “if the process memory reaches 2.3GB, flush all caches and reduce the project cache size to 1”. Or even enable it by default.

@vasily-kirichenko
Copy link
Contributor

On this PR error highlighting does not update until the project is built:

1

(I'm not sure if the bug is introduced in this PR, I've not tested VFP for about a month or more).

@vasily-kirichenko
Copy link
Contributor

Also, "go to definition" does not work for FSharpChecker.Create from FSharp.Editor project:

image

@dsyme
Copy link
Contributor Author

dsyme commented Jul 14, 2017

On this PR error highlighting does not update until the project is built:

With in-memory cross-project references enabled or disabled?

@dsyme
Copy link
Contributor Author

dsyme commented Jul 14, 2017

On this PR error highlighting does not update until the project is built:

I think this is #3047

@vasily-kirichenko
Copy link
Contributor

vasily-kirichenko commented Jul 14, 2017

With in-memory cross-project references enabled or disabled?

Disabled.

@dsyme
Copy link
Contributor Author

dsyme commented Jul 14, 2017

Closing in favour of #3330

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants