-
Notifications
You must be signed in to change notification settings - Fork 783
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
Transparent Compiler memory reduction in editors #17543
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -559,7 +559,7 @@ and [<Experimental("This FCS API is experimental and subject to change.")>] FSha | |||||
referencedProjects: FSharpReferencedProjectSnapshot list, | ||||||
isIncompleteTypeCheckEnvironment: bool, | ||||||
useScriptResolutionRules: bool, | ||||||
loadTime: DateTime, | ||||||
loadTime: DateTime, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the space? |
||||||
unresolvedReferences: FSharpUnresolvedReferencesSet option, | ||||||
originalLoadReferences: (range * string * string) list, | ||||||
stamp: int64 option | ||||||
|
@@ -675,27 +675,32 @@ and [<Experimental("This FCS API is experimental and subject to change.")>] FSha | |||||
|> async.Return | ||||||
|
||||||
FSharpProjectSnapshot.FromOptions(options, getFileSnapshot) | ||||||
|
||||||
let internal snapshotTable = ConditionalWeakTable<ProjectSnapshot, FSharpProjectOptions>() | ||||||
|
||||||
let rec internal snapshotToOptions (projectSnapshot: ProjectSnapshot) = | ||||||
snapshotTable.GetValue(projectSnapshot, fun projectSnapshot -> | ||||||
{ | ||||||
ProjectFileName = projectSnapshot.ProjectFileName | ||||||
ProjectId = projectSnapshot.ProjectId | ||||||
SourceFiles = projectSnapshot.SourceFiles |> Seq.map (fun x -> x.FileName) |> Seq.toArray | ||||||
OtherOptions = projectSnapshot.CommandLineOptions |> List.toArray | ||||||
ReferencedProjects = | ||||||
projectSnapshot.ReferencedProjects | ||||||
|> Seq.map (function | ||||||
| FSharpReference(name, opts) -> FSharpReferencedProject.FSharpReference(name, opts.ProjectSnapshot |> snapshotToOptions) | ||||||
| PEReference(getStamp, reader) -> FSharpReferencedProject.PEReference(getStamp, reader) | ||||||
| ILModuleReference(name, getStamp, getReader) -> FSharpReferencedProject.ILModuleReference(name, getStamp, getReader)) | ||||||
|> Seq.toArray | ||||||
IsIncompleteTypeCheckEnvironment = projectSnapshot.IsIncompleteTypeCheckEnvironment | ||||||
UseScriptResolutionRules = projectSnapshot.UseScriptResolutionRules | ||||||
LoadTime = projectSnapshot.LoadTime | ||||||
UnresolvedReferences = projectSnapshot.UnresolvedReferences | ||||||
OriginalLoadReferences = projectSnapshot.OriginalLoadReferences | ||||||
Stamp = projectSnapshot.Stamp | ||||||
} | ||||||
Comment on lines
+680
to
+702
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the main fix around creating too many options There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes sense. Out of curiosity what are you using the resulting options for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There seems to be paths that still want a Specifically they show up in transparent compiler which get turned into longer lived objects here:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see, didn't notice these usages. Wonder what would be a good way to phase them out of there... But for now at least your fix improves it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that would the real fix. I did do before that but it was more invasive. |
||||||
) | ||||||
|
||||||
let rec internal snapshotToOptions (projectSnapshot: ProjectSnapshotBase<_>) = | ||||||
{ | ||||||
ProjectFileName = projectSnapshot.ProjectFileName | ||||||
ProjectId = projectSnapshot.ProjectId | ||||||
SourceFiles = projectSnapshot.SourceFiles |> Seq.map (fun x -> x.FileName) |> Seq.toArray | ||||||
OtherOptions = projectSnapshot.CommandLineOptions |> List.toArray | ||||||
ReferencedProjects = | ||||||
projectSnapshot.ReferencedProjects | ||||||
|> Seq.map (function | ||||||
| FSharpReference(name, opts) -> FSharpReferencedProject.FSharpReference(name, opts.ProjectSnapshot |> snapshotToOptions) | ||||||
| PEReference(getStamp, reader) -> FSharpReferencedProject.PEReference(getStamp, reader) | ||||||
| ILModuleReference(name, getStamp, getReader) -> FSharpReferencedProject.ILModuleReference(name, getStamp, getReader)) | ||||||
|> Seq.toArray | ||||||
IsIncompleteTypeCheckEnvironment = projectSnapshot.IsIncompleteTypeCheckEnvironment | ||||||
UseScriptResolutionRules = projectSnapshot.UseScriptResolutionRules | ||||||
LoadTime = projectSnapshot.LoadTime | ||||||
UnresolvedReferences = projectSnapshot.UnresolvedReferences | ||||||
OriginalLoadReferences = projectSnapshot.OriginalLoadReferences | ||||||
Stamp = projectSnapshot.Stamp | ||||||
} | ||||||
|
||||||
[<Extension>] | ||||||
type internal Extensions = | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -258,45 +258,119 @@ module private TypeCheckingGraphProcessing = | |
|
||
return finalFileResults, state | ||
} | ||
|
||
type internal CompilerCaches(sizeFactor: int) = | ||
type CacheSizes = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably should be internal There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allows an editor to finely tweak these options. This potentially trades memory for CPU usage but on larger solutions it a tradeoff someone can make it possible to have multiple solutions open. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, how would the editor come up with these numbers? I wouldn't suggest to expose this to the user. I think you should be able to say "use more memory" or "use less memory", potentially even "hold more/less stuff strongly/weakly" but does it really make sense to have such granular control in the editor? So that different editors would benefit from different configurations somehow? And if there is some smart way to set these based the solutions the user is working here it would be nice to have that code here so everyone can use it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I exposed these numbers so it was easier to try to adjust them individually but I do agree it's not something I'd want to expose to a user directly. I think the only settings come down to "memory" vs "recacluating". Specifically, my work project can be around 10-15gb once fully type checked, if I have multiple versions of open for various reasons, I'd rather try to be aggressive with collecting and keeping it's profile small. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we can expose it now for easier experimenting with some |
||
ParseFileKeepStrongly: int | ||
ParseFileKeepWeakly: int | ||
ParseFileWithoutProjectKeepStrongly: int | ||
ParseFileWithoutProjectKeepWeakly: int | ||
ParseAndCheckFileInProjectKeepStrongly: int | ||
ParseAndCheckFileInProjectKeepWeakly: int | ||
ParseAndCheckAllFilesInProjectKeepStrongly: int | ||
ParseAndCheckAllFilesInProjectKeepWeakly: int | ||
ParseAndCheckProjectKeepStrongly: int | ||
ParseAndCheckProjectKeepWeakly: int | ||
FrameworkImportsKeepStrongly: int | ||
FrameworkImportsKeepWeakly: int | ||
BootstrapInfoStaticKeepStrongly: int | ||
BootstrapInfoStaticKeepWeakly: int | ||
BootstrapInfoKeepStrongly: int | ||
BootstrapInfoKeepWeakly: int | ||
TcLastFileKeepStrongly: int | ||
TcLastFileKeepWeakly: int | ||
TcIntermediateKeepStrongly: int | ||
TcIntermediateKeepWeakly: int | ||
DependencyGraphKeepStrongly: int | ||
DependencyGraphKeepWeakly: int | ||
ProjectExtrasKeepStrongly: int | ||
ProjectExtrasKeepWeakly: int | ||
AssemblyDataKeepStrongly: int | ||
AssemblyDataKeepWeakly: int | ||
SemanticClassificationKeepStrongly: int | ||
SemanticClassificationKeepWeakly: int | ||
ItemKeyStoreKeepStrongly: int | ||
ItemKeyStoreKeepWeakly: int | ||
ScriptClosureKeepStrongly: int | ||
ScriptClosureKeepWeakly: int | ||
} | ||
with | ||
static member Create sizeFactor = | ||
|
||
{ | ||
ParseFileKeepStrongly = 50 * sizeFactor | ||
ParseFileKeepWeakly = 20 * sizeFactor | ||
ParseFileWithoutProjectKeepStrongly = 5 * sizeFactor | ||
ParseFileWithoutProjectKeepWeakly = 2 * sizeFactor | ||
ParseAndCheckFileInProjectKeepStrongly = sizeFactor | ||
ParseAndCheckFileInProjectKeepWeakly = 2 * sizeFactor | ||
ParseAndCheckAllFilesInProjectKeepStrongly = sizeFactor | ||
ParseAndCheckAllFilesInProjectKeepWeakly = 2 * sizeFactor | ||
ParseAndCheckProjectKeepStrongly = sizeFactor | ||
ParseAndCheckProjectKeepWeakly = 2 * sizeFactor | ||
FrameworkImportsKeepStrongly = sizeFactor | ||
FrameworkImportsKeepWeakly = 2 * sizeFactor | ||
BootstrapInfoStaticKeepStrongly = sizeFactor | ||
BootstrapInfoStaticKeepWeakly = 2 * sizeFactor | ||
BootstrapInfoKeepStrongly = sizeFactor | ||
BootstrapInfoKeepWeakly = 2 * sizeFactor | ||
TcLastFileKeepStrongly = sizeFactor | ||
TcLastFileKeepWeakly = 2 * sizeFactor | ||
TcIntermediateKeepStrongly = 20 * sizeFactor | ||
TcIntermediateKeepWeakly = 20 * sizeFactor | ||
DependencyGraphKeepStrongly = sizeFactor | ||
DependencyGraphKeepWeakly = 2 * sizeFactor | ||
ProjectExtrasKeepStrongly = sizeFactor | ||
ProjectExtrasKeepWeakly = 2 * sizeFactor | ||
AssemblyDataKeepStrongly = sizeFactor | ||
AssemblyDataKeepWeakly = 2 * sizeFactor | ||
SemanticClassificationKeepStrongly = sizeFactor | ||
SemanticClassificationKeepWeakly = 2 * sizeFactor | ||
ItemKeyStoreKeepStrongly = sizeFactor | ||
ItemKeyStoreKeepWeakly = 2 * sizeFactor | ||
ScriptClosureKeepStrongly = sizeFactor | ||
ScriptClosureKeepWeakly = 2 * sizeFactor | ||
} | ||
|
||
static member Default = | ||
let sizeFactor = 100 | ||
CacheSizes.Create sizeFactor | ||
type internal CompilerCaches(sizeFactor: CacheSizes) = | ||
|
||
let sf = sizeFactor | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be renamed if we keep it like this. |
||
|
||
member _.SizeFactor = sf | ||
member _.CacheSizes = sf | ||
|
||
member val ParseFile = AsyncMemoize(keepStrongly = 50 * sf, keepWeakly = 20 * sf, name = "ParseFile") | ||
member val ParseFile = AsyncMemoize(keepStrongly = sf.ParseFileKeepStrongly, keepWeakly = sf.ParseFileKeepWeakly, name = "ParseFile") | ||
|
||
member val ParseFileWithoutProject = | ||
AsyncMemoize<string, string, FSharpParseFileResults>(keepStrongly = 5 * sf, keepWeakly = 2 * sf, name = "ParseFileWithoutProject") | ||
AsyncMemoize<string, string, FSharpParseFileResults>(sf.ParseFileWithoutProjectKeepStrongly, keepWeakly = sf.ParseFileWithoutProjectKeepWeakly, name = "ParseFileWithoutProject") | ||
|
||
member val ParseAndCheckFileInProject = AsyncMemoize(sf, 2 * sf, name = "ParseAndCheckFileInProject") | ||
member val ParseAndCheckFileInProject = AsyncMemoize(sf.ParseAndCheckFileInProjectKeepStrongly, sf.ParseAndCheckFileInProjectKeepWeakly, name = "ParseAndCheckFileInProject") | ||
|
||
member val ParseAndCheckAllFilesInProject = AsyncMemoizeDisabled(sf, 2 * sf, name = "ParseAndCheckFullProject") | ||
member val ParseAndCheckAllFilesInProject = AsyncMemoizeDisabled(sf.ParseAndCheckAllFilesInProjectKeepStrongly, sf.ParseAndCheckAllFilesInProjectKeepWeakly, name = "ParseAndCheckFullProject") | ||
|
||
member val ParseAndCheckProject = AsyncMemoize(sf, 2 * sf, name = "ParseAndCheckProject") | ||
member val ParseAndCheckProject = AsyncMemoize(sf.ParseAndCheckProjectKeepStrongly, sf.ParseAndCheckProjectKeepWeakly, name = "ParseAndCheckProject") | ||
|
||
member val FrameworkImports = AsyncMemoize(sf, 2 * sf, name = "FrameworkImports") | ||
member val FrameworkImports = AsyncMemoize(sf.FrameworkImportsKeepStrongly, sf.FrameworkImportsKeepWeakly, name = "FrameworkImports") | ||
|
||
member val BootstrapInfoStatic = AsyncMemoize(sf, 2 * sf, name = "BootstrapInfoStatic") | ||
member val BootstrapInfoStatic = AsyncMemoize(sf.BootstrapInfoStaticKeepStrongly, sf.BootstrapInfoStaticKeepWeakly, name = "BootstrapInfoStatic") | ||
|
||
member val BootstrapInfo = AsyncMemoize(sf, 2 * sf, name = "BootstrapInfo") | ||
member val BootstrapInfo = AsyncMemoize(sf.BootstrapInfoKeepStrongly, sf.BootstrapInfoKeepWeakly, name = "BootstrapInfo") | ||
|
||
member val TcLastFile = AsyncMemoizeDisabled(sf, 2 * sf, name = "TcLastFile") | ||
member val TcLastFile = AsyncMemoizeDisabled(sf.TcLastFileKeepStrongly, sf.TcLastFileKeepWeakly, name = "TcLastFile") | ||
|
||
member val TcIntermediate = AsyncMemoize(20 * sf, 20 * sf, name = "TcIntermediate") | ||
member val TcIntermediate = AsyncMemoize(sf.TcIntermediateKeepStrongly, sf.TcIntermediateKeepWeakly, name = "TcIntermediate") | ||
|
||
member val DependencyGraph = AsyncMemoize(sf, 2 * sf, name = "DependencyGraph") | ||
member val DependencyGraph = AsyncMemoize(sf.DependencyGraphKeepStrongly, sf.DependencyGraphKeepWeakly, name = "DependencyGraph") | ||
|
||
member val ProjectExtras = AsyncMemoizeDisabled(sf, 2 * sf, name = "ProjectExtras") | ||
member val ProjectExtras = AsyncMemoizeDisabled(sf.ProjectExtrasKeepStrongly, sf.ProjectExtrasKeepWeakly, name = "ProjectExtras") | ||
|
||
member val AssemblyData = AsyncMemoize(sf, 2 * sf, name = "AssemblyData") | ||
member val AssemblyData = AsyncMemoize(sf.AssemblyDataKeepStrongly, sf.AssemblyDataKeepWeakly, name = "AssemblyData") | ||
|
||
member val SemanticClassification = AsyncMemoize(sf, 2 * sf, name = "SemanticClassification") | ||
member val SemanticClassification = AsyncMemoize(sf.SemanticClassificationKeepStrongly, sf.SemanticClassificationKeepWeakly, name = "SemanticClassification") | ||
|
||
member val ItemKeyStore = AsyncMemoize(sf, 2 * sf, name = "ItemKeyStore") | ||
member val ItemKeyStore = AsyncMemoize(sf.ItemKeyStoreKeepStrongly, sf.ItemKeyStoreKeepWeakly, name = "ItemKeyStore") | ||
|
||
member val ScriptClosure = AsyncMemoize(sf, 2 * sf, name = "ScriptClosure") | ||
member val ScriptClosure = AsyncMemoize(sf.ScriptClosureKeepStrongly, sf.ScriptClosureKeepWeakly, name = "ScriptClosure") | ||
|
||
member this.Clear(projects: Set<ProjectIdentifier>) = | ||
let shouldClear project = projects |> Set.contains project | ||
|
@@ -327,7 +401,8 @@ type internal TransparentCompiler | |
captureIdentifiersWhenParsing, | ||
getSource: (string -> Async<ISourceText option>) option, | ||
useChangeNotifications, | ||
useSyntaxTreeCache | ||
useSyntaxTreeCache, | ||
?cacheSizes | ||
) as self = | ||
|
||
let documentSource = | ||
|
@@ -337,9 +412,11 @@ type internal TransparentCompiler | |
|
||
// Is having just one of these ok? | ||
let lexResourceManager = Lexhelp.LexResourceManager() | ||
|
||
let cacheSizes = defaultArg cacheSizes CacheSizes.Default | ||
|
||
// Mutable so we can easily clear them by creating a new instance | ||
let mutable caches = CompilerCaches(100) | ||
let mutable caches = CompilerCaches(cacheSizes) | ||
|
||
// TODO: do we need this? | ||
//let maxTypeCheckingParallelism = max 1 (Environment.ProcessorCount / 2) | ||
|
@@ -1363,8 +1440,8 @@ type internal TransparentCompiler | |
node, | ||
(fun tcInfo -> | ||
|
||
if tcInfo.stateContainsNodes |> Set.contains fileNode then | ||
failwith $"Oops!" | ||
// if tcInfo.stateContainsNodes |> Set.contains fileNode then | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like an unrelated fix. Not sure if it's been discussed elsewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Been discussed in slack before (probably beyond any history now 😢 ). Yeah this could be reverted but I've seen this popup crashing the TC hard. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. It seems a bit fishy. I need to understand if this is something that should legitimately be happening, clearly previously I thought it shouldn't 😅 |
||
// failwith $"Oops!" | ||
|
||
//if | ||
// tcInfo.stateContainsNodes | ||
|
@@ -1409,14 +1486,8 @@ type internal TransparentCompiler | |
fileNode, | ||
(fun tcInfo -> | ||
|
||
if tcInfo.stateContainsNodes |> Set.contains fileNode then | ||
failwith $"Oops!" | ||
|
||
// if | ||
// tcInfo.stateContainsNodes | ||
// |> Set.contains (NodeToTypeCheck.PhysicalFile(index + 1)) | ||
// then | ||
// failwith $"Oops!!!" | ||
// if tcInfo.stateContainsNodes |> Set.contains fileNode then | ||
// failwith $"Oops!" | ||
|
||
let parsedInput = projectSnapshot.SourceFiles[index].ParsedInput | ||
let prefixPathOpt = None | ||
|
@@ -1672,6 +1743,7 @@ type internal TransparentCompiler | |
cacheKey.GetKey(), | ||
(fun (_fullVersion, fileContentVersion) -> fileContentVersion = (snd version)) | ||
) | ||
|
||
|
||
match parseFileResultsAndcheckFileAnswer with | ||
| Some(parseFileResults, FSharpCheckFileAnswer.Succeeded checkFileResults) -> Some(parseFileResults, checkFileResults) | ||
|
@@ -2079,9 +2151,13 @@ type internal TransparentCompiler | |
|
||
member _.Caches = caches | ||
|
||
member _.SetCacheSizeFactor(sizeFactor: int) = | ||
if sizeFactor <> caches.SizeFactor then | ||
caches <- CompilerCaches(sizeFactor) | ||
member _.SetCacheSize(cacheSize : CacheSizes) = | ||
if cacheSize <> caches.CacheSizes then | ||
caches <- CompilerCaches(cacheSize) | ||
|
||
member x.SetCacheSizeFactor(sizeFactor : int) = | ||
let newCacheSize = CacheSizes.Create sizeFactor | ||
x.SetCacheSize newCacheSize | ||
|
||
interface IBackgroundCompiler with | ||
|
||
|
@@ -2143,7 +2219,7 @@ type internal TransparentCompiler | |
|
||
member _.ClearCaches() : unit = | ||
backgroundCompiler.ClearCaches() | ||
caches <- CompilerCaches(100) // TODO: check | ||
caches <- CompilerCaches(cacheSizes) // TODO: check | ||
|
||
member _.DownsizeCaches() : unit = backgroundCompiler.DownsizeCaches() | ||
|
||
|
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 FSAC InternalsVisibleTo to allow the cache tweaks
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 feels very wrong, is this a temporary thing?
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 transparent compiler itself is marked internal and unless the F# team wants to make it public, this is a better method than forcing FSAC to do reflection all the time.
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.
Yeah, that's true, but there's actually a way to enable it using
FSharpChecker.Create(useTransparentCompiler = true)
. Shouldn't more things be driven by that?I just think this approach feels like a workaround that avoids proper communication, which seems like a missed opportunity. But if the team doesn't see it that way and everyone's fine with it, then I suppose it's okay.
One other thing that concerns me is that this could potentially be a common request for every FCS user.
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 wonder if we should expose everything before we stabilize it? Or, we can expose necessary parts, but put
Experimental
on them. Or expose them via the checker?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 don't have a strong preference here. I went with the least invasive changes first to do exactly this, facilitate discussion :)
Yeah I agree here. Though, it would be nice to also be able to access other things like LanguageFeatures instead of our hack
Since FCS makes breaking changes that FSAC needs to handle anyway, it's all kind of the same thing in the end to me. Whatever makes more sense for your team.
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.
We'd be happy to have such
InternalsVisibleTo
entries too in Rider plugin. 🙂We're also used to the changes in APIs.
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.
In this case we should just expose everything needed via APIs.
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 would also propose to add [Experimental] APIs for everything you need. Then at least we see everything that's needed when deciding on a stable API.