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

FsiEvaluationSession.Create does not unload FSI-ASSEMBLY when collectible = true #15669

Open
maciej-izak opened this issue Jul 24, 2023 · 11 comments
Labels
Area-FSI Bug Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code.
Milestone

Comments

@maciej-izak
Copy link

Succinct description

When collectible = true is passed as an argument to FsiEvaluationSession.Create, the FSI-ASSEMBLY is never unloaded, contrary to expectations.

Repro steps

The problem can be reproduced by following these steps:

  1. Run the provided collectionTest function in the Collectible code generation section of the FCS API documentation: https://fsharp.github.io/fsharp-compiler-docs/fcs/interactive.html#Collectible-code-generation
  2. Observe that the FSI-ASSEMBLY is never unloaded.

Expected behavior

When collectible = true is set, the FSI-ASSEMBLY should be unloaded when it's no longer needed.

Actual behavior

The FSI-ASSEMBLY is never unloaded, even when collectible = true.

Known workarounds

There are currently no known workarounds for this issue.

Potential solution

A potential fix might involve modifying the defineDynamicAssemblyAndLog function in ilreflect.fs to ensure that the assembly is created within a CollectibleAssemblyLoadContext. Here's the current implementation of the function:

let defineDynamicAssemblyAndLog (asmName, flags, asmDir: string) =
    let asmB = AssemblyBuilder.DefineDynamicAssembly(asmName, flags)

    if logRefEmitCalls then
        printfn "open System"
        printfn "open System.Reflection"
        printfn "open System.Reflection.Emit"

        printfn
            "let assemblyBuilder%d = System.AppDomain.CurrentDomain.DefineDynamicAssembly(AssemblyName(Name=\"%s\"), enum %d, %A)"
            (abs <| hash asmB)
            asmName.Name
            (LanguagePrimitives.EnumToValue flags)
            asmDir

    asmB

Related information

Operating system: Windows 10 Pro 21H2 (19044.3086)
.NET Runtime kind: .NET 7.0.306
Editing Tools: Visual Studio Code

@github-actions github-actions bot added this to the Backlog milestone Jul 24, 2023
@0101 0101 added Area-FSI Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code. and removed Needs-Triage labels Jul 24, 2023
@0101
Copy link
Contributor

0101 commented Jul 24, 2023

Can you share what the use case for this is?

@maciej-izak
Copy link
Author

@0101 Certainly! My primary use case involves a form designer for the Avalonia framework using NXUI.FSharp package for F#, where each form is maintained in the form of an F# file. Every form is loaded with a separate instance of FSI and the memory usage quickly increases, especially when the collectible functionality doesn't work as intended.

More generally, this kind of functionality can be quite useful in scenarios where we need to compile and execute code dynamically during runtime. This includes scripting engines, runtime evaluations for mathematical expressions, or custom business rules engines. In these cases, we need to compile and run user-defined logic on the fly. Using collectible assemblies helps ensure that the dynamically generated and loaded code can be unloaded, and the memory freed when it's no longer needed. This allows us to manage memory more effectively in situations where a large amount of dynamic code is being generated and discarded.

Therefore, having this issue addressed is crucial to enhancing the efficiency and versatility of F# in a wide range of applications, including but not limited to the primary use case I mentioned above.

@vzarytovskii
Copy link
Member

@0101 Certainly! My primary use case involves a form designer for the Avalonia framework using NXUI.FSharp package for F#, where each form is maintained in the form of an F# file. Every form is loaded with a separate instance of FSI and the memory usage quickly increases, especially when the collectible functionality doesn't work as intended.

More generally, this kind of functionality can be quite useful in scenarios where we need to compile and execute code dynamically during runtime. This includes scripting engines, runtime evaluations for mathematical expressions, or custom business rules engines. In these cases, we need to compile and run user-defined logic on the fly. Using collectible assemblies helps ensure that the dynamically generated and loaded code can be unloaded, and the memory freed when it's no longer needed. This allows us to manage memory more effectively in situations where a large amount of dynamic code is being generated and discarded.

Therefore, having this issue addressed is crucial to enhancing the efficiency and versatility of F# in a wide range of applications, including but not limited to the primary use case I mentioned above.

That makes sense, we have already planned current work, but will accept PR, would you like to take a shot at it?

There are couple of things to keep in mind though - by default, we emit multiple assemblies at time (the multiemit option), it's done so they can be debugged, so probably they will also need to be loaded using custom ALC.

@maciej-izak
Copy link
Author

@vzarytovskii Thank you for your response and your openness to accepting a PR for this issue. I appreciate the complexity of this problem, especially with the multiemit option and the potential implications for debugging.

Currently, my project is in an early development phase and my bandwidth is quite limited due to other ongoing projects. Although this issue is significant to me and may potentially become critical in the future, I'm unable to commit to contributing a PR at this moment.

@vzarytovskii
Copy link
Member

@vzarytovskii Thank you for your response and your openness to accepting a PR for this issue. I appreciate the complexity of this problem, especially with the multiemit option and the potential implications for debugging.

Currently, my project is in an early development phase and my bandwidth is quite limited due to other ongoing projects. Although this issue is significant to me and may potentially become critical in the future, I'm unable to commit to contributing a PR at this moment.

We'll what we can do, when we wrap up our work for .NET 8 release.

@maciej-izak
Copy link
Author

maciej-izak commented Sep 13, 2023

Request for Priority Reevaluation: FsiEvaluationSession.Create Unloading Issue

@0101 @vzarytovskii I'd like to bring attention to the significance of the issue regarding the non-unloading behavior of FSI-ASSEMBLY with collectible = true. I believe the current categorization as low-impact might not fully capture the potential implications for a broad range of F# applications. Here are my reasons:

Memory Management & Performance: This issue results in a potential memory leak due to the persistent retention of collectible assemblies. Such suboptimal memory management can deteriorate the performance, especially in applications with frequent dynamic compilations.

Documentation Consistency: The observed behavior directly contradicts the documented expectations. Ensuring alignment between documentation and actual functionality is crucial for the developer's trust in a platform.

Lack of Alternatives: Without known workarounds, developers are left stranded, potentially hampering or halting development in projects reliant on this feature.

Real-world Impact: As demonstrated in my primary use case involving the Avalonia framework's form designer, this issue can have tangible effects on ongoing projects, impacting not just developers but end-users as well.

F#'s Versatility: Addressing this issue can solidify F#'s reputation as a versatile tool capable of handling a range of dynamic scenarios effectively.

Given the above arguments, I kindly request a reevaluation of this ticket's priority, considering its upgrade from low-impact to either medium or high impact. This would be greatly beneficial for all developers relying on this feature, ensuring the robustness and trustworthiness of F#.

Thank you for your consideration.

SilkyFowl added a commit to SilkyFowl/Avalonia.FuncUI.LiveView that referenced this issue Oct 30, 2023
…roject

- Generated assembly is no longer locked, allowing Preview target project to be built.
- The assembly is automatically reloaded when it is updated.
- Currently, changes to the path of referenced or generated assemblies themselves cannot be detected. We would like to detect them if possible, but we will not address this issue at this time.

Changes:
- Add `PrijectAssemblyWatcher` class to project assembly and load or reload assembly dynamically.
  - To avoid building the project, read assembly bytes from byte[] instead of directly from the file.
  - If the watched assembly is updated, remove it from the cache. The updated assembly will be loaded again when it is requested after being removed from the cache.
  - Add `resolvingHandler` to replace loaded project assembly.
- Update `Service` class to use `PrijectAssemblyWatcher` class.

TODO:
- Implement `Unload` method to unload assembly manually after [dotnetfsharp/#15669](dotnet/fsharp#15669) is fixed.
@maciej-izak
Copy link
Author

@vzarytovskii it is planned for .NET 9?

@vzarytovskii
Copy link
Member

vzarytovskii commented Feb 22, 2024

@vzarytovskii it is planned for .NET 9?

No, it isn't explicitly planned, we are at the capacity currently. If someone from the team or community will look at it and fix it, we'll include it, but it's not part of any of our F# 9 buckets.

@vzarytovskii
Copy link
Member

vzarytovskii commented Feb 22, 2024

Looking at the issue, I'm not entirely sure how is it going to work with support of multi emit on coreclr. Can you please try disabling multi emit and see if it unloads the assembly (--multiemit- in the arguments when creating the session)?

We don't use dynamic assembly builders when we use multi emit:

let builders =
        if tcConfigB.fsiMultiAssemblyEmit then
            None
        else
            let assemBuilder, moduleBuilder =
                mkDynamicAssemblyAndModule (dynamicCcuName, tcConfigB.optSettings.LocalOptimizationsEnabled, fsiCollectible)

            dynamicAssemblies.Add(assemBuilder)
            Some(assemBuilder, moduleBuilder)

Update: like so:

open System.IO
open FSharp.Compiler.Interactive.Shell
let collectionTest () =

    for i in 1 .. 200 do
        let defaultArgs =
            [| "fsi.exe"
               "--noninteractive"
               "--nologo"
               "--multiemit-" // <- this flag
               "--gui-" |]

        use inStream = new StringReader("")
        use outStream = new StringWriter()
        use errStream = new StringWriter()

        let fsiConfig =
            FsiEvaluationSession.GetDefaultConfiguration()

        use session =
            FsiEvaluationSession.Create(fsiConfig, defaultArgs, inStream, outStream, errStream, collectible = true)

        session.EvalInteraction(sprintf "type D = { v : int }")

        let v =
            session.EvalExpression(sprintf "{ v = 42 * %d }" i)

        printfn "iteration %d, result = %A" i v.Value.ReflectionValue

collectionTest ()

When evaluating this particular example, I don't see any increase in the memory consumption

@vzarytovskii
Copy link
Member

On the second thought, it might be not depending on the dynamic assemblies, since it uses AppDomain to load/unload assemblies, which is not supported in SDK, if that's the case, this will require a rewrite using some alternative mechanism.

@vzarytovskii
Copy link
Member

I time boxed potential fix here: #16748, not sure how to test it. Any help will be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-FSI Bug Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants