dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.06k stars 4.04k forks source link

CSharpScript.EvaluateAsync() growing memory usage #31751

Open stefanoIT opened 5 years ago

stefanoIT commented 5 years ago

Version Used: 2.10.0

Steps to Reproduce:

It's quite easy to reproduce, just run in Loop the following code:

string formula = "Math.Round((double)(15/(double)10*100),2)"; object result = await CSharpScript.EvaluateAsync(formula, o_scriptoptions);

Expected Behavior: I was not expecting an ever-growing memory usage.

Actual Behavior:

The behaviour is well described by the following pictures from our memory profiler, showing live instances and bytes:

image

image

The issue might be related to the following:

22219

10164

rummelsworth commented 4 years ago

This hit an app I'm working on pretty hard. We run long "analysis" operations and were trying to run hundreds at once when we ran into this. Our expressions are numeric-only and simple enough that we easily transitioned to Jace.NET to avoid this memory leak in Roslyn, but we've got another app in which expressions can have string parameters and operations, which will prevent usage of Jace.NET.

I was about to create a new issue for this, so I'll add what I had already written:

Version Used: Microsoft.CodeAnalysis.CSharp.Scripting 3.4.0. Also tried 3.5.0-beta3-final.

Steps to Reproduce:

  1. Create a new .NET Core 3.1 console app.

  2. Install the Microsoft.CodeAnalysis.CSharp.Scripting NuGet package.

  3. Replace the contents of Program.cs with this:

    using Microsoft.CodeAnalysis.CSharp.Scripting;
    using System;
    
    namespace ConsoleApp1
    {
       internal class Program
       {
           private static void Main(string[] args)
           {
               Once();
               Once();
           }
    
           private static void Once()
           {
               Console.WriteLine();
               GC.Collect();
               Console.WriteLine("before: " + GC.GetTotalMemory(true));
    
               const string EXPRESSION = "123";
               int result;
    
               using (var task = CSharpScript.EvaluateAsync<int>(EXPRESSION))
               {
                   result = task.Result;
               }
    
               Console.WriteLine("  " + result);
               GC.Collect();
               Console.WriteLine("after: " + GC.GetTotalMemory(true));
           }
       }
    }
  4. Set breakpoints at line 11 (after the first Once invocation) and 12 (after the second Once invocation).

  5. Run the app under Debug.

  6. When the first breakpoint hits, ensure the Diagnostic Tools window is open, and take a memory usage snapshot.

  7. Let it continue to the next breakpoint, and then take another memory snapshot.

Expected Behavior:

No increase in allocated objects/memory.

Actual Behavior:

Some increase in allocated objects/memory:

image

MaxiPigna commented 4 years ago

Any update to share on this issue?

guylevy commented 3 years ago

Any update on this issue which is open since almost 3 years?

dpalat commented 1 year ago

Any update on this issue which is open since almost 5 years?

CyrusNajmabadi commented 1 year ago

The item is in the backlog. If you would like to contribute a fix, let us know. Thanks!

zh6335901 commented 2 months ago

Any update on this issue which is open since almost 6 years?

CyrusNajmabadi commented 2 months ago

@zh6335901 see previous message.

zh6335901 commented 2 months ago

@zh6335901 see previous message.

Yes, I would like to contribute a fix.

Through issues 41722 and 72366 as well as the source code, I have identified that the primary cause of the memory leak is that each time a script is run, the InteractiveAssemblyLoader is used to load the assemblies generated by the script. The InteractiveAssemblyLoader internally uses private LoadContext that inherits AssemblyLoadContext to load assemblies (in .NET Core). However, this LoadContext cannot be unloaded because its Collectible property is set to false by default. As a result, when you continuously run different scripts, memory usage keeps increasing.

Based on the above, I propose the following solution:

  1. The InteractiveAssemblyLoader should provide an isCollectible parameter and pass it to the internal LoadContext. Then, in the Dispose method, attempt to unload the LoadContext:

    
    public sealed class InteractiveAssemblyLoader
    {
    public InteractiveAssemblyLoader(MetadataShadowCopyProvider? shadowCopyProvider = null, bool isCollectible = false)
    {
        ...other irrelevant code
        _runtimeAssemblyLoader = AssemblyLoaderImpl.Create(this, isCollectible);
    }
    
    public void Dispose()
    {
        // This field reference loaded assemblies, so it must be cleared before the loader is disposed.
        // Otherwise, AssemblyLoadContext.Unload would not working
        _loadedAssembliesBySimpleName.Clear();
        _runtimeAssemblyLoader.Dispose(); 
    }
    }

...

internal sealed class CoreAssemblyLoaderImpl : AssemblyLoaderImpl { private readonly LoadContext _inMemoryAssemblyContext;

internal CoreAssemblyLoaderImpl(InteractiveAssemblyLoader loader, bool isCollectible)
    : base(loader)
{
    _inMemoryAssemblyContext = new LoadContext(Loader, null, isCollectible);
}

public override void Dispose()
{
    if (_inMemoryAssemblyContext.IsCollectible)
    {
        _inMemoryAssemblyContext.Unload();
    }
}

private sealed class LoadContext : AssemblyLoadContext
{
    internal LoadContext(InteractiveAssemblyLoader loader, string? loadDirectory, bool isCollectible) : base(isCollectible)
    {
        ...Some initialization code
    }
}

}



2. Another solution would be to expose `AssemblyLoaderImpl` (which may need to be renamed) and allow it to be publicly inherited. This would enable users to customize the behavior of loading assemblies and make unloading possible.

@CyrusNajmabadi Do you have any suggestion for this? thanks:)
CyrusNajmabadi commented 2 months ago

@CyrusNajmabadi Do you have any suggestion for this? thanks:)

I don't know this space. But that seems like a reasonable place to start to see how it works out. If your results work out for you, and you don't see any negative behavior, we can review your change. Tomas would be a good person to start looking at it once he's back from vacation. Thanks :)

zh6335901 commented 2 months ago

@CyrusNajmabadi Do you have any suggestion for this? thanks:)

I don't know this space. But that seems like a reasonable place to start to see how it works out. If your results work out for you, and you don't see any negative behavior, we can review your change. Tomas would be a good person to start looking at it once he's back from vacation. Thanks :)

I have tried first solution that add isCollectible pararmeter, It results work out for my demo. I will create a pull request later.