dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.9k stars 782 forks source link

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

Open maciej-izak opened 1 year ago

maciej-izak commented 1 year ago

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

0101 commented 1 year ago

Can you share what the use case for this is?

maciej-izak commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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.

maciej-izak commented 8 months ago

@vzarytovskii it is planned for .NET 9?

vzarytovskii commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

I time boxed potential fix here: https://github.com/dotnet/fsharp/pull/16748, not sure how to test it. Any help will be appreciated.