Create the FSharpChecker like VFPT does - fixes #1974 #2124
Conversation
|
Great! :) |
|
What does memory/CPU consumption look like with this change? |
| let checker = | ||
| lazy | ||
| let checker = FSharpChecker.Create() | ||
| let checker = FSharpChecker.Create(projectCacheSize = 200, keepAllBackgroundResolutions = false) |
There was a problem hiding this comment.
FWIW I've always been very skeptical about this huge project cache size used by VFPT. It is much, much, much bigger than that used previously by Visual F# Tools (3 projects).
I know @vasily-kirichenko thinks differently though - but I just wanted to highlight that this setting doesn't feel instinctively right to me, and my gut intuition is that it will lead to OOM in VS too often. We need a methodology to test operational memory usage on typical scenarios such as 5-10 very large projects, or hundreds of small projects.
Some information on Roslyn's choices here may also be useful - how much semantic analysis information does Roslyn keep live, and what policies/techniques are used to manage that.
There was a problem hiding this comment.
Also, I would very much like to see this setting be user-settable, or at least controlled by an environment variable or something. Given the major features added in VS2017, I feel it's inevitable that we will get reports that the F# support now uses more memory (and hence becomes unusable more often). We really need to be able to do on-machine diagnosis and treatment when that happens.
There was a problem hiding this comment.
(I suppose the immediately safe thing is to remove the 200 setting - since there's discussion about whether it's a definite improvement in all scenarios, whereas keepAllBackgroundResolutions is agreed to always be an improvement).
There was a problem hiding this comment.
@dsyme in https://github.com/Microsoft/visualfsharp/blob/ee84eb77bcd62379d8178e3167a17d12d204de30/src/fsharp/vs/service.fs I see things like this:
GetEnvInteger "FCS_ProjectCacheSizeDefault" 3Does that mean that we can override the value with environment variables?
I think at least it would be a way to tweak those settings if situation becomes dire, although with this PR this is not possible for that 200 setting.
There was a problem hiding this comment.
@smoothdeveloper No, that setting is the one used if none is provided explicitly at FSharpChecker creation. So this changes the setting from 3 to 200 (which means basically strongly holding all background checkers and associated resources for all projects - which intuitively feels sure to blow up memory usage with many scripts or projects - in F# each script counts as a project)
There was a problem hiding this comment.
@dsyme I meant if we revert that parameter change in the PR, can we use environment variable to override it?
Also, longer term, would making stuff that project relies on (like the same assembly metadata, which is probably retrieved for many projects in a solution) shared across the board have a significant impact on memory used?
There was a problem hiding this comment.
@dsyme In VFPT, we provided an option in settings dialogs to override project cache size (default 50) https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/src/FSharpVSPowerTools/UI/GlobalOptionsPage.cs#L29-L33
@cartermp We very badly need proper methodology for evaluating the memory-performance impact of new VS features. Advice from the Roslyn team on how they achieve automated memory performance testing may be useful here. |
|
@KevinRansom I feel we may need to slow down the rate of merging so we can discuss and assess things like #2124 (review) before integrating. |
|
From my (a couple of years I think) experience with that parameter I can say that it definitely does not help with memory consumption, but makes FCS unusable if it's set to a small number. If you set it to, say, 50 and try to check 60 projects, it will take it ages to finish (if ever). On the other hand, the compiler project can easily consume all the 2.5GB in minutes, so this setting should be something like 'number of lines of checked code', not even 'number of checked files', let alone projects. |
|
@vasily-kirichenko Yeah, agreed. Or just an overall "maximum memory for F# project caching". I wish it was easier to measure how much those caches are consuming |
8 participants
fixes #1974
/cc @vasily-kirichenko