Fix the memory surge caused by script running and not being GC controlled for a long time

[fanslead]

When there are a large number of C # scripts, frequent compilation and running of scripts can cause memory to skyrocket and not be released by GC for a long time.It will cause the server to run out of memory and crash.
By pointing scriptState to a null reference to break the reference relationship, it can be released by GC as early as possible, thereby reducing memory resource consumption.

---

This change is 

[Suchiman]

To my knowledge, this changes effectively nothing. scriptState is a local variable, not a field, it stops existing the moment the method returns which happens immediately after returning ReturnValue.

[fanslead]

To my knowledge, this changes effectively nothing. scriptState is a local variable, not a field, it stops existing the moment the method returns which happens immediately after returning ReturnValue.

@Suchiman I have tested it before, before adding this operation,It will cause the server to run out of memory and crash. GC is in effect, but it seems that the situation of freeing memory is not good.

By pointing scriptState to a null reference to break the reference relationship.During each GC, a large amount of memory can be released

[sfmskywalker]

@fanslead Are you sure the difference in GC activity is caused by setting the local variable scriptState to null? Because that really makes no sense given how GC works - at least, how I think I know it works. The local variable will cease to exist as soon as the method returns. What would be more logical is if the caller of that method holds on to the returned value.
Perhaps you are able to explain how nullifying a local variable has this effect? My fear is that it seems to have an effect, but that there's another factor at play that causes the different results that you're seeing.

[sfmskywalker]

@fanslead A more logical explanation for the increase in memory consumption is the fact that the scripts are cached. The larger the number of scripts, the larger the amount of scripts being stored in cache. To see if this is the case, maybe try and see what happens when you update the following method:

private Script<object> GetPreCompiledScript(Script<object> script)
{
    var cacheKey = "csharp:script:" + Hash(script);

    return memoryCache.GetOrCreate(cacheKey, entry =>
    {
        if (_csharpOptions.ScriptCacheTimeout.HasValue)
            entry.SetSlidingExpiration(_csharpOptions.ScriptCacheTimeout.Value);

        return script;
    })!;
}

Remove the caching logic:

private Script<object> GetPreCompiledScript(Script<object> script)
{
    return script;
}

[sfmskywalker]

@Suchiman I just noticed that the caching of the Script object might be ineffective, because the script is always created, and then cached:

private Script<object> GetPreCompiledScript(Script<object> script)
    {
        var cacheKey = "csharp:script:" + Hash(script);

        return memoryCache.GetOrCreate(cacheKey, entry =>
        {
            if (_csharpOptions.ScriptCacheTimeout.HasValue)
                entry.SetSlidingExpiration(_csharpOptions.ScriptCacheTimeout.Value);

            return script;
        })!;
    }

Notice that the script object is simply passed in as an argument, and then added to te cache before being returned.
Is this on purpose, and if so, how does this work exactly? Unless there's some magic going on, it would seem to me that the script object is created every time a script is being evaluated, rather than the script being fetched from cache only.

[fanslead]

@fanslead Are you sure the difference in GC activity is caused by setting the local variable scriptState to null? Because that really makes no sense given how GC works - at least, how I think I know it works. The local variable will cease to exist as soon as the method returns. What would be more logical is if the caller of that method holds on to the returned value. Perhaps you are able to explain how nullifying a local variable has this effect? My fear is that it seems to have an effect, but that there's another factor at play that causes the different results that you're seeing.

@sfmskywalker When by pointing scriptState to a null reference and Immediately execute GC forced recycling. Memory can be stably released. But immediate GC will lead to increased CPU consumption.

var scriptState = await script.RunAsync(globals, cancellationToken: cancellationToken);

var returnValue = scriptState.ReturnValue;

scriptState = null;
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();

But,When I do not change any. The server memory quickly fills up and crashes

[fanslead]

@fanslead A more logical explanation for the increase in memory consumption is the fact that the scripts are cached. The larger the number of scripts, the larger the amount of scripts being stored in cache. To see if this is the case, maybe try and see what happens when you update the following method:

private Script<object> GetPreCompiledScript(Script<object> script)
{
    var cacheKey = "csharp:script:" + Hash(script);

    return memoryCache.GetOrCreate(cacheKey, entry =>
    {
        if (_csharpOptions.ScriptCacheTimeout.HasValue)
            entry.SetSlidingExpiration(_csharpOptions.ScriptCacheTimeout.Value);

        return script;
    })!;
}

Remove the caching logic:

private Script<object> GetPreCompiledScript(Script<object> script)
{
    return script;
}

I try caching the script. Of course, it is effective, but if there are a large number of different scripts and the memory is not released for a long time, it will also fill up the server's memory and cause it to crash. Meanwhile, when running with cache, there will also be a slight increase in memory, about 5-10MB. If cache is not used, the memory growth during each compilation and execution of the script is approximately 200Mb-300Mb

[sfmskywalker]

Ok. I'd like to do some experiments as well. In order to reproduce the memory consumption, what's the number of C# scripts we're looking at, give or take?

[Suchiman]

Is this on purpose, and if so, how does this work exactly? Unless there's some magic going on, it would seem to me that the script object is created every time a script is being evaluated, rather than the script being fetched from cache only.

That is correct, i would have had to rewrite the entire scripting engine to make it not depend on the Script<object> object directly, so i exploited the fact, that Script is immutable and only does the compilation on the first call to RunAsync which is then cached inside the Script<object>. When the code calls script = GetPreCompiledScript(script);, the first time around, the script will be added to the cache and then it returns that same input object, which will be compiled on that following RunAsync. The second time it comes around, it takes the script, finds a similiar one in the cache and then returns the cached script instead which was previously run and thus previously compiled.

[Suchiman]

@fanslead

If cache is not used, the memory growth during each compilation and execution of the script is approximately 200Mb-300Mb

Caching of the scripts is a very recent addition (new in 3.3-RC3, released 14h ago), was the leak observed with or without the caching? Not caching the scripts might actually lead to memory exhaustion, because i'm not sure, if the dynamically generated assemblies from the scripting engine are ever freed. IIRC they didn't use to, because roslyn couldn't generate collectible assemblies so they stuck around forever, but i don't know if that changed.

[fanslead]

@fanslead

If cache is not used, the memory growth during each compilation and execution of the script is approximately 200Mb-300Mb

Caching of the scripts is a very recent addition (new in 3.3-RC3, released 14h ago), was the leak observed with or without the caching? Not caching the scripts might actually lead to memory exhaustion, because i'm not sure, if the dynamically generated assemblies from the scripting engine are ever freed. IIRC they didn't use to, because roslyn couldn't generate collectible assemblies so they stuck around forever, but i don't know if that changed.

@Suchiman I think caching scripts can certainly suppress memory growth to a certain extent. However, cached scripts will not be released by GC, and when the number of different scripts reaches a certain level, the memory will still be full.The best solution should be to ensure that the script is promptly reclaimed by GC.Of course, my PR plan can only slightly accelerate the timing of script recycling by GC

[fanslead]

@Suchiman @sfmskywalker I made a mistake. The server environment and VS Debug environment are different. When I force GC in VS, the memory can be stably released, but in the server environment, this effect doesn't look good.
I also tried caching scripts. I used a 2C8G server and with only 3 C # scripts, the memory usage after caching reached 40%.