Many tweaks to AdaptiveLSPServer for performance #1024
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This updates GetLastWriteTimeShim to use actualFs.GetLastWriteTimeShim which should help with how the type checker recognizes doing caching for type checks. Additionally updates DidOpen, DidChange, and DidSave to use the file systems GetLastWriteTimeUtc to further help with this type check caching.
TheAngryByrd
force-pushed
the
minor-adaptive-tweaks-2
branch
from
a38d775 to
0a184c5
Compare
TheAngryByrd
commented
Oct 11, 2022
Comment on lines
+358
to
+403
| /// <summary>Updates the Lines value</summary> | ||
| member this.SetLines(lines) = { this with Lines = lines } | ||
|
|
||
| /// <summary>Updates the Lines value with supplied text</summary> | ||
| member this.SetText(text) = | ||
| this.SetLines(NamedText(this.Lines.FileName, text)) | ||
|
|
||
|
|
||
| /// <summary>Updates the Touched value</summary> | ||
| member this.SetTouched touched = { this with Touched = touched } | ||
|
|
||
|
|
||
| /// <summary>Updates the Touched value attempting to use the file on disk's GetLastWriteTimeUtc otherwise uses DateTime.UtcNow. </summary> | ||
| member this.UpdateTouched() = | ||
| let path = UMX.untag this.Lines.FileName | ||
|
|
||
| let dt = | ||
| if File.Exists path then | ||
| File.GetLastWriteTimeUtc path | ||
| else | ||
| DateTime.UtcNow | ||
|
|
||
| this.SetTouched dt | ||
|
|
||
|
|
||
| /// <summary>Helper method to create a VolatileFile</summary> | ||
| static member Create(lines, version, touched) = | ||
| { Lines = lines | ||
| Version = version | ||
| Touched = touched } | ||
|
|
||
| /// <summary>Helper method to create a VolatileFile</summary> | ||
| static member Create(path, text, version, touched) = | ||
| VolatileFile.Create(NamedText(path, text), version, touched) | ||
|
|
||
| /// <summary>Helper method to create a VolatileFile, attempting to use the file on disk's GetLastWriteTimeUtc otherwise uses DateTime.UtcNow.</summary> | ||
| static member Create(path: string<LocalPath>, text, version) = | ||
| let touched = | ||
| let path = UMX.untag path | ||
|
|
||
| if File.Exists path then | ||
| File.GetLastWriteTimeUtc path | ||
| else | ||
| DateTime.UtcNow | ||
|
|
||
| VolatileFile.Create(path, text, version, touched) |
There was a problem hiding this comment.
Lots of helper functions for creating VolatileFiles
| @@ -412,7 +460,7 @@ type FileSystem(actualFs: IFileSystem, tryFindFile: string<LocalPath> -> Volatil | |||
| |> Utils.normalizePath | |||
| |> tryFindFile | |||
| |> Option.map (fun f -> f.Touched) | |||
| |> Option.defaultValue DateTime.MinValue | |||
| |> Option.defaultWith (fun () -> actualFs.GetLastWriteTimeShim f) | |||
There was a problem hiding this comment.
This is the change the should fix typecheck caching issues
There was a problem hiding this comment.
oh geez, this would have impacted everyone, huh?
| @@ -105,7 +105,7 @@ module AVal = | |||
| /// <summary> | |||
| /// Calls a mapping function which creates additional dependencies to be tracked. | |||
| /// </summary> | |||
| let mapWithAdditionalDependenies (mapping: 'a -> 'b * list<IAdaptiveValue>) (value: aval<'a>) : aval<'b> = | |||
| let mapWithAdditionalDependenies (mapping: 'a -> 'b * list<#IAdaptiveValue>) (value: aval<'a>) : aval<'b> = | |||
There was a problem hiding this comment.
Minor change to avoid needing callsites to cast
Comment on lines
+621
to
+630
| let cancelAllOpenFileCheckRequests () = | ||
| transact (fun () -> | ||
| let files = openFiles |> AMap.force | ||
|
|
||
| for (_, fileVal) in files do | ||
| let (oldFile, cts: CancellationTokenSource) = fileVal |> AVal.force | ||
| cts.Cancel() | ||
| cts.Dispose() | ||
| fileVal.Value <- oldFile, new CancellationTokenSource()) | ||
|
|
There was a problem hiding this comment.
Calls cancel on all openfiles's CancellationTokenSource to stop type checking.
Comment on lines
+826
to
+827
| NotificationEvent.ParseError(errors, file.Lines.FileName) | ||
| |> notifications.Trigger |
There was a problem hiding this comment.
ParseError just sets the diagnostics collection the same way so reusing the event.
| @@ -1548,16 +1547,10 @@ type AdaptiveFSharpLspServer(workspaceLoader: IWorkspaceLoader, lspClient: FShar | |||
|
|
|||
| let doc = p.TextDocument | |||
| let filePath = doc.GetFilePath() |> Utils.normalizePath | |||
| let file = volaTileFile filePath doc.Text (Some doc.Version) DateTime.UtcNow | |||
| // We want to try to use the file system's datetime if available | |||
| let file = VolatileFile.Create(filePath, doc.Text, (Some doc.Version)) | |||
There was a problem hiding this comment.
Callsite change for creating a VolatileFile with the filesystems utc value.
| } | ||
| ) | ||
|
|
||
| forceGetTypeCheckResults filePath |> ignore |
There was a problem hiding this comment.
Instead of just waiting around, lets start type checking since we'll cancel it on the next request anyway
Comment on lines
+1630
to
+1638
| let file = | ||
| option { | ||
| let! oldFile = forceFindOpenFile filePath | ||
| let oldFile = p.Text |> Option.map (oldFile.SetText) |> Option.defaultValue oldFile | ||
| return oldFile.UpdateTouched() | ||
| } | ||
| |> Option.defaultWith (fun () -> | ||
| // Very unlikely to get here | ||
| VolatileFile.Create(filePath, p.Text.Value, None, DateTime.UtcNow)) |
There was a problem hiding this comment.
Lots of fumbling to try to be safe around DidSave and updating a VolatileFile correctly
| |> Async.withCancellationSafe (fun () -> cts.Token) | ||
| |> Async.Ignore<unit option> | ||
|
|
||
| do! lspClient.CodeLensRefresh() |
There was a problem hiding this comment.
Moves CodeLensRefresh to DidSave to avoid ionide from sending too many requests.
| @@ -188,7 +188,7 @@ module ObservableExtensions = | |||
|
|
|||
| /// Fires an event only after the specified interval has passed in which no other pending event has fired. Buffers all events leading up to that emit. | |||
| member x.BufferedDebounce(ts: TimeSpan) = | |||
| x.Publish(fun shared -> shared.Window(shared.Throttle(ts))) | |||
| x.Publish(fun shared -> shared.Window(shared.Throttle(ts))).SelectMany(fun l -> l.ToList()) | |||
There was a problem hiding this comment.
The fix to actually put the windows into buffers
this was processing observable windows as they came in instead of waiting for them the close.
TheAngryByrd
force-pushed
the
minor-adaptive-tweaks-2
branch
from
00fd659 to
ea846b5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
lspClient.CodeLensRefresh()toDidSaveinstead of every typecheckThanks to @AngelMunoz and @PaigeM89 for pairing with me on these.