fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

Transparent Compiler memory reduction in editors

Open TheAngryByrd opened this issue 1 year ago • 2 comments

Description

This fixes #16979 with some additions that have been discussed in various chats/twitter threads. I've combined them for discussion purposes, we can split them up as desired.

The 3 parts are:

  1. Makes the snapshots -> options use a ConditionalWeakTable rather than recreating possibly hundreds of options objects.
  2. Adds many various knobs to the transparent compiler caching code, allowing tweaking for editors.
  3. Adds FsAutocomplete to the list of InternalsVisibleTo so we can utilize these cache tweaking changes.
    • FCS has never bee backwards compatible and FSAC has always had to deal with source level changes so any concerns around versioning wouldn't really apply here. We're used to it 😄

❓ Outstanding questions ❓

  • What type of benchmarking should I do for this?
    • I could show before/after on allocations in FSAC.
    • Add a benchmark dotnet scenario but I'm unsure how big a project graph needs to be before this exhibits the behavior.

Checklist

  • [ ] Test cases added

  • [ ] Performance benchmarks added in case of performance changes

  • [ ] Release notes entry updated:

    Please make sure to add an entry with short succinct description of the change as well as link to this pull request to the respective release notes file, if applicable.

    Release notes files:

    • If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/.FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
    • If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/releae-notes/.Language/preview.md
    • If a change to FSharp.Core was made, please make sure to edit docs/release-notes/.FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.

    Information about the release notes entries format can be found in the documentation. Example:

    If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

TheAngryByrd avatar Aug 15 '24 12:08 TheAngryByrd

:heavy_exclamation_mark: Release notes required


:white_check_mark: Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.200.md

github-actions[bot] avatar Aug 15 '24 12:08 github-actions[bot]

I could show before/after on allocations in FSAC.

Yeah, that's good enough. Also, if possible, CPU consumption.

vzarytovskii avatar Aug 16 '24 14:08 vzarytovskii

In terms of why I want to get this fix in:

My work project is around 70 projects. This is the use after a full typecheck of the solution in FSAC:

Background Compiler: image

Transparent Compiler: Screenshot 2024-12-08 135219 (Actually this wasn't a full typecheck because I ran out of memory and it wouldn't finish)

Transparent Compiler (with this PR): Screenshot 2024-12-08 135833

TheAngryByrd avatar Dec 08 '24 19:12 TheAngryByrd

I can't seem to run the ilverify.ps1 script

& : The module 'fsharp' could not be loaded. For more information, run 'Import-Module fsharp'.
At C:\Users\jimmy\Repositories\public\TheAngryByrd\fsharp\tests\ILVerify\ilverify.ps1:41 char:7
+     & $script -c $configuration $additional_arguments
+       ~~~~~~~
    + CategoryInfo          : ObjectNotFound: (fsharp\build.sh:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CouldNotAutoLoadModule

TheAngryByrd avatar Dec 08 '24 19:12 TheAngryByrd

I can't seem to run the ilverify.ps1 script


& : The module 'fsharp' could not be loaded. For more information, run 'Import-Module fsharp'.

At C:\Users\jimmy\Repositories\public\TheAngryByrd\fsharp\tests\ILVerify\ilverify.ps1:41 char:7

+     & $script -c $configuration $additional_arguments

+       ~~~~~~~

    + CategoryInfo          : ObjectNotFound: (fsharp\build.sh:String) [], CommandNotFoundException

    + FullyQualifiedErrorId : CouldNotAutoLoadModule

It requires pwsh, not powershell (i.e. version 7, not 5).

vzarytovskii avatar Dec 09 '24 07:12 vzarytovskii

In terms of why I want to get this fix in:

[...]

Wow all of that because of the extra FSharpOptions 😲 Or did you tweak the cache sizes also?

0101 avatar Dec 10 '24 09:12 0101

AWESOME!

baronfel avatar Jan 06 '25 14:01 baronfel

Thank for merging this.

edgarfgp avatar Jan 06 '25 14:01 edgarfgp

@T-Gro do you have any idea what SDK release this will make it into? Is this branch still inserting into 9.0.200?

baronfel avatar Jan 06 '25 14:01 baronfel

❤️ Thank you for finishing this up!

TheAngryByrd avatar Jan 06 '25 15:01 TheAngryByrd

@T-Gro do you have any idea what SDK release this will make it into? Is this branch still inserting into 9.0.200?

fsharp's main is not inserting into 17.13 automatically anymore, but Petr did one last manual merge to get this in -> it should still land in 9.0.200

T-Gro avatar Jan 06 '25 15:01 T-Gro

@T-Gro do you have any idea what SDK release this will make it into? Is this branch still inserting into 9.0.200?

fsharp's main is not inserting into 17.13 automatically anymore, but Petr did one last manual merge to get this in -> it should still land in 9.0.200

That's great news :) FSAC is considering a nightly release to get just this feature because I was worried that we'd have to wait until 9.0.300.

baronfel avatar Jan 06 '25 15:01 baronfel