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

Reduce excess memory usage in TransparentCompiler #17543

Merged
merged 15 commits into from
Jan 6, 2025

Conversation

TheAngryByrd
Copy link
Contributor

@TheAngryByrd TheAngryByrd commented Aug 15, 2024

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.

❓ 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.

Copy link
Contributor

github-actions bot commented Aug 15, 2024

❗ Release notes required


✅ 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

@TheAngryByrd TheAngryByrd changed the title Snapshot fixes Transparent Compiler memory reduction in editors Aug 15, 2024
src/Compiler/Service/TransparentCompiler.fs Outdated Show resolved Hide resolved
src/Compiler/Service/FSharpProjectSnapshot.fs Outdated Show resolved Hide resolved
src/Compiler/FSharp.Compiler.Service.fsproj Outdated Show resolved Hide resolved
src/Compiler/Service/FSharpProjectSnapshot.fs Outdated Show resolved Hide resolved
@vzarytovskii
Copy link
Member

I could show before/after on allocations in FSAC.

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

src/Compiler/Service/TransparentCompiler.fs Outdated Show resolved Hide resolved
src/Compiler/Service/TransparentCompiler.fs Outdated Show resolved Hide resolved
src/Compiler/FSharp.Compiler.Service.fsproj Outdated Show resolved Hide resolved
src/Compiler/Service/TransparentCompiler.fs Outdated Show resolved Hide resolved
src/Compiler/Service/FSharpProjectSnapshot.fs Outdated Show resolved Hide resolved
@TheAngryByrd
Copy link
Contributor Author

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 TheAngryByrd changed the title Transparent Compiler memory reduction in editors Reduce excess memory usage in TransparentCompiler Dec 8, 2024
@TheAngryByrd TheAngryByrd marked this pull request as ready for review December 8, 2024 19:15
@TheAngryByrd TheAngryByrd requested a review from a team as a code owner December 8, 2024 19:15
@TheAngryByrd
Copy link
Contributor Author

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

@vzarytovskii
Copy link
Member

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).

Copy link
Member

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

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

The results look really good Jimmy!

I would want to get this in soon to start using it, could you please resolve the deleted "failwith $"Oops!"" code and changing the "sf" naming to reflect new reality?

@0101
Copy link
Contributor

0101 commented Dec 10, 2024

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?

@T-Gro T-Gro enabled auto-merge (squash) December 10, 2024 11:48
@T-Gro T-Gro merged commit fbdb7cb into dotnet:main Jan 6, 2025
33 checks passed
@baronfel
Copy link
Member

baronfel commented Jan 6, 2025

AWESOME!

@edgarfgp
Copy link
Contributor

edgarfgp commented Jan 6, 2025

Thank for merging this.

@baronfel
Copy link
Member

baronfel commented Jan 6, 2025

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

@TheAngryByrd
Copy link
Contributor Author

❤️ Thank you for finishing this up!

@T-Gro
Copy link
Member

T-Gro commented Jan 6, 2025

@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

@baronfel
Copy link
Member

baronfel commented Jan 6, 2025

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Transparent Compiler: High memory usage in FSAC
8 participants