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

Reducing memory allocations for file path maths #1139

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

TheAngryByrd
Copy link
Member

@TheAngryByrd TheAngryByrd commented Jul 11, 2023

WHAT

🤖 Generated by Copilot at 3612b5b

This pull request updates the FSharp.Compiler.Service library and introduces some performance and robustness improvements to the FsAutoComplete project. It removes some obsolete code, adds some error logging, optimizes some string and buffer operations, and enhances the language server protocol communication. It also adds two new NuGet packages: LinkDotNet.StringBuilder and CommunityToolkit.HighPerformance. It includes an unrelated change to the playground.fsx file that may need to be removed or explained.

🤖 Generated by Copilot at 3612b5b

To make FsAutoComplete faster and leaner
They added some packages and cleaned up the FSharpCompilerServiceChecker
They used spans and pools for strings and buffers
And tweaked the AdaptiveFSharpLspServer
Now it's a more efficient and robust code completer

🚀🐛🧹

WHY

These get called all the time, even while scrolling. While it's not a ton of savings compared to toher calls, it adds up. Didn't get rid of the URI LocalPath allocation because that's way to much work while this was a relatively simple switch to spans.

image image

HOW

🤖 Generated by Copilot at 3612b5b

  • Remove unnecessary thread switching in compiler service methods (link, link, link, link, link, link, link, link)
  • Add error logging to TryGetRecentCheckResultsForFile method (link)
  • Add LinkDotNet.StringBuilder and CommunityToolkit.HighPerformance packages to improve performance and memory efficiency (link, link, link)
  • Use ReadOnlySpan<char> and ValueStringBuilder types to avoid string allocations and optimize path and uri manipulation in Utils.fs (link, link, link, link)
  • Use SpanOwner type to manage memory buffers in AdaptiveFSharpLspServer.fs (link)
  • Use custom ThrottleBy extension method to group and delay analyzer execution in AdaptiveFSharpLspServer.fs (link)
  • Modify log level and context in parseAndCheckFile function in AdaptiveFSharpLspServer.fs (link)
  • Avoid redundant async operation in forceGetTypeCheckResultsStale function in AdaptiveFSharpLspServer.fs (link)
  • Delete temporary file after executing command in handleCommandEvents function in AdaptiveFSharpLspServer.fs (link)
  • Comment out debug logging in FileSystem.fs to reduce verbosity and string allocations (link, link)
  • Add code to playground.fsx for testing custom computation expression builder (link)

@TheAngryByrd TheAngryByrd force-pushed the allocations branch 4 times, most recently from 15c9893 to 56c9c90 Compare July 14, 2023 01:28
@TheAngryByrd TheAngryByrd marked this pull request as ready for review July 14, 2023 01:28
@TheAngryByrd TheAngryByrd force-pushed the allocations branch 2 times, most recently from f4c2410 to b09e1aa Compare July 15, 2023 14:50
Comment on lines +838 to +840
// fsLogger.debug (
// Log.setMessage "{path} expanded to {expanded}"
// >> Log.addContext "path" f
// >> Log.addContext "expanded" expanded
// )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These logging functions can get called a lot and do actually allocate so commented out for now.

We'll work on some lower allocation solution later.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let inline debug a b c = ()

let foo () =
  debug 1 (2+2) 4

it seems the compiler is optimizing this, wondering if this can be used to make the debug function a no-op in release builds while keeping the code around for debug builds?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess no:

let inline debug a b c = ()

let foo () =
  debug (new obj()) (new obj()) (new obj())

it doesn't get elided.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could always ifdef them i guess

@TheAngryByrd TheAngryByrd force-pushed the allocations branch 2 times, most recently from 3b14e1b to 2717429 Compare July 15, 2023 18:14
Copy link
Contributor

@baronfel baronfel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@baronfel baronfel merged commit f97f9de into ionide:main Jul 28, 2023
9 checks passed
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.

3 participants