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

Make in memory assemblies in InteractiveAssemblyLoader can be collected when use scripting #74915

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

zh6335901
Copy link

See #31751 and #41722

Add isCollectible parameter to InteractiveAssemblyLoader makes collecting assemblies that in memory possible.

My test code:

while (true)
{
    var loader = new InteractiveAssemblyLoader(isCollectible: true);
    var options = ScriptOptions.Default
        .AddReferences(MetadataReference.CreateFromFile(typeof(TestClass).Assembly.Location))
        .WithImports("System", "ScriptingClassLibrary");

    var script = CSharpScript.Create("new TestClass().ToJson(1)", options, assemblyLoader: loader);
    var result = await script.RunAsync().ConfigureAwait(false);
    result = await result.ContinueWithAsync("1 + 2").ConfigureAwait(false);
    result = await result.ContinueWithAsync("new TestClass().ToJson(\"aaa\")").ConfigureAwait(false);
    result = await result.ContinueWithAsync("new TestClass().ToJson(false)").ConfigureAwait(false);
    result = await result.ContinueWithAsync("1 + 3").ConfigureAwait(false);

    loader.Dispose();

    GC.Collect();
    GC.WaitForPendingFinalizers();

    await Task.Delay(100).ConfigureAwait(false);

    var currentProcess = Process.GetCurrentProcess();
    Console.WriteLine($"Process handle count: {currentProcess.HandleCount}");
}

Test result:
image
img_v3_02e5_36303847-acc1-4d97-b3e1-7f7b9f5b394g

You can see memory usage and process handle count is steady.

But same test code(without isCollectible parameter) use Microsoft.CodeAnalysis.CSharp.Scripting 4.12.0-1.final package:

img_v3_02e5_e224f46c-e27f-44f2-ad41-8f2f31318b3g
img_v3_02e5_1200b2b7-f776-4897-b0b3-6a5e5e8300dg

@zh6335901 zh6335901 requested a review from a team as a code owner August 27, 2024 14:59
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Interactive untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 27, 2024
@dotnet-policy-service dotnet-policy-service bot added Community The pull request was submitted by a contributor who is not a Microsoft employee. Needs API Review Needs to be reviewed by the API review council labels Aug 27, 2024
@zh6335901
Copy link
Author

@zh6335901 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

}

public void Dispose()
{
// This field reference loaded assemblies, so it must be cleared before the loader is disposed.
// Otherwise, AssemblyLoadContext.Unload in CoreAssemblyLoaderImpl would not working
_loadedAssembliesBySimpleName.Clear();
Copy link
Author

@zh6335901 zh6335901 Aug 27, 2024

Choose a reason for hiding this comment

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

I didn't find any place to invoke this Dispose method in roslyn repo, I don't sure it's safe or suitable to clear _loadedAssembliesBySimpleName , But in my test case i don't see any negative behavior

@zh6335901
Copy link
Author

Anyone can review it?

@DraconInteractive
Copy link

Curious to whether this is still applicable? Running into the problem of assemblies hanging around in memory on the latest branch of ros, this seems like a solid solution to me. Hope it gets bumped for review!

@meetsekhar
Copy link

meetsekhar commented Nov 1, 2024

Could you please let me know if this fix is working and is available? We’re experiencing a production issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interactive Community The pull request was submitted by a contributor who is not a Microsoft employee. Needs API Review Needs to be reviewed by the API review council untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants