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

[DRAFT] Featuring MemoryManager to resolve issues related to native memory leaking #613

Draft
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

KillStr3aK
Copy link
Contributor

@KillStr3aK KillStr3aK commented Oct 7, 2024

Hey there, I propose this as a 'demo' currently. It works but not sure if this is the right implementation for this on the managed side, so I'm open for any ideas or extensions.

Currently every instance of Vector/QAngle/Angle creates a new pointer in unmanaged that is stored but never released, therefore it leaks memory which has been known since they were implemented.

Is it a breaking change?

The current implementation that involves IDisposableMemory/DisposableMemory should not be a breaking change as the types are still NativeObject, DisposableMemory is more like a proxy type to correctly release the resources.

How it works currently

When we access a value through the schema system, usually we instantiate the managed class and pass the pure game pointer to it so we can access the elements under the hood. Since we should not release these, I've added a PurePointer property that is only set to true when we explicitly mark the instance as pure.

Instances that are not marked as pure are should be safe to release, and we enforce these to be collected by the GC through the IMemoryManager that runs indefinitely (in a separate thread) by an interval that can be set in the CoreConfig. Once an instance is collected by the garbage collector, the finalizer will call the corresponding native to release it on the unmanaged side.

Since these are collected by the garbage collector itself, of course only the unused instances will be collected. Even if we don't use IMemoryManager (lets say we don't implement that part, or just disable it via CoreConfig) these will be collected, we just don't know when.

Anything in the schemasystem that is Span<T> (#134) OR NetworkedVector<T> (#135) (where T is Vector, QAngle, Quaternion, Vector2D, Vector4D, etc) cannot be used so currently the implementation for those are not that important (I kinda made it tho, but could not test)

Benchmark

I have no performance benchmarks in production with forcing GC yet, also we might change CoreConfig.MemoryManagerInterval to a viable interval, it was only used for tests.

However I tried to mass release instances and I could release 100.000 vector pointers in ~234ms without the server crashing, or disconnecting any players, however there were lags but I'd say it was related to the IO operations and the massive native calls.

Syntax adjustments

Since these types become IDisposable this syntax is now allowed:

using (Vector vector = new Vector(150.0f, 150.0f, 150.0f))
{
    // work with the vector here
}

// `vector` instance is now released automatically without GC

Releasing Vector instances:

image

// Instances like these are not supposed to be collected, and they won't be as long as its stored/not changed. Otherwise the GC will clean it up
public Vector ThisShouldNotBeGarbageCollected = new Vector();

image

MemAlloc

Also features the MemAlloc class with exposed natives.

public static class MemAlloc
{
    /// <summary>
    /// Indirect allocation using 'MemAlloc'
    /// </summary>
    /// <param name="size"></param>
    /// <returns></returns>
    public static nint Allocate(int size)
    {
        return NativeAPI.MemAllocAllocate(size);
    }

    /// <summary>
    /// Indirect allocation using 'MemAlloc'
    /// </summary>
    /// <param name="size"></param>
    /// <param name="align"></param>
    /// <returns></returns>
    public static nint AllocateAligned(int size, int align)
    {
        return NativeAPI.MemAllocAllocateAligned(size, align);
    }

    public static nint ReallocateAligned(nint pointer, int size, int align)
    {
        return NativeAPI.MemAllocReallocateAligned(pointer, size, align);
    }

    public static int GetSizeAligned(nint pointer)
    {
        return NativeAPI.MemAllocGetSizeAligned(pointer);
    }

    /// <summary>
    /// Release pointer using 'MemAlloc'
    /// </summary>
    /// <param name="ptr"></param>
    public static void Free(nint ptr)
    {
        NativeAPI.MemAllocFreePointer(ptr);
    }

    /// <summary>
    /// Release pointer using 'MemAlloc'
    /// </summary>
    /// <param name="ptr"></param>
    public static void FreeAligned(nint ptr)
    {
        NativeAPI.MemAllocFreePointerAligned(ptr);
    }
}

I will continue working on this, and once we get this one done we can implement CEntityKeyValues (#615)

From the minimal and limited tests I've done I can say that this whole system could work

Note

I would not log the memory manager related stuff as 'INFO' but 'TRACE'

Comment on lines +26 to +62
<None Remove="Modules\Commands\CommandInfo" />
<None Remove="Modules\Disabled\**" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="McMaster.NETCore.Plugins" Version="1.4.0"/>
<PackageReference Include="Microsoft.CSharp" Version="4.7.0"/>
<PackageReference Include="Microsoft.DotNet.ApiCompat.Task" Version="8.0.203"/>
<PackageReference Include="Microsoft.Extensions.Hosting" Version="8.0.0"/>
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" Version="8.0.0"/>
<PackageReference Include="Microsoft.Extensions.Localization.Abstractions" Version="8.0.3"/>
<PackageReference Include="Microsoft.Extensions.Logging" Version="8.0.0"/>
<PackageReference Include="Scrutor" Version="4.2.2"/>
<PackageReference Include="Serilog.Extensions.Logging" Version="8.0.0"/>
<PackageReference Include="Serilog.Sinks.Console" Version="5.0.0"/>
<PackageReference Include="Serilog.Sinks.File" Version="5.0.0"/>
<PackageReference Include="System.Data.DataSetExtensions" Version="4.5.0"/>
<PackageReference Include="McMaster.NETCore.Plugins" Version="1.4.0" />
<PackageReference Include="Microsoft.CSharp" Version="4.7.0" />
<PackageReference Include="Microsoft.DotNet.ApiCompat.Task" Version="8.0.203" />
<PackageReference Include="Microsoft.Extensions.Hosting" Version="8.0.0" />
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" Version="8.0.0" />
<PackageReference Include="Microsoft.Extensions.Localization.Abstractions" Version="8.0.3" />
<PackageReference Include="Microsoft.Extensions.Logging" Version="8.0.0" />
<PackageReference Include="Scrutor" Version="4.2.2" />
<PackageReference Include="Serilog.Extensions.Logging" Version="8.0.0" />
<PackageReference Include="Serilog.Sinks.Console" Version="5.0.0" />
<PackageReference Include="Serilog.Sinks.File" Version="5.0.0" />
<PackageReference Include="System.Data.DataSetExtensions" Version="4.5.0" />
</ItemGroup>
<ItemGroup>
<Folder Include="Core\Schema\"/>
<Folder Include="Modules\Errors"/>
<Folder Include="Core\Schema\" />
<Folder Include="Modules\Errors" />
</ItemGroup>
<ItemGroup>
<Compile Remove="Modules\Disabled\**"/>
<Compile Remove="Modules\Disabled\**" />
</ItemGroup>
<ItemGroup>
<EmbeddedResource Remove="Modules\Disabled\**"/>
<EmbeddedResource Remove="Modules\Disabled\**" />
</ItemGroup>
<PropertyGroup/>
<PropertyGroup />
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|AnyCPU'">
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|AnyCPU'">
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>
<Target Name="SetSourceRevisionId" BeforeTargets="InitializeSourceControlInformation">
<Exec
Command="git describe --long --always --exclude=* --abbrev=7"
ConsoleToMSBuild="True"
IgnoreExitCode="False"
>
<Output PropertyName="SourceRevisionId" TaskParameter="ConsoleOutput"/>
<Exec Command="git describe --long --always --exclude=* --abbrev=7" ConsoleToMSBuild="True" IgnoreExitCode="False">
<Output PropertyName="SourceRevisionId" TaskParameter="ConsoleOutput" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDE probably auto formatted it, I'm not sticking to this change haha, can be reversed

Comment on lines 263 to 325
private void BackgroundThread()
{
_logger.LogInformation("Service has been started");
State = MemoryManagerState.Idle;

while (State != MemoryManagerState.Stopped)
{
Thread.Sleep(CoreConfig.MemoryManagerInterval);

if (State == MemoryManagerState.Stopped)
break;

if (State == MemoryManagerState.Halted)
continue;

int totalCount = CurrentResources;

if (totalCount == 0)
continue;

State = MemoryManagerState.InProgress;

_logger.LogInformation("Running garbage collector ({0} disposable memory in total)", totalCount);
DateTime startTime = DateTime.UtcNow;

// some may go to gen1 or even gen2, but even those are released when this nondeterministic wonder wants so
GC.Collect(0, GCCollectionMode.Default, true);

// this might be obsolete with 'blocking: false'?
GC.WaitForPendingFinalizers();

// this part is wrong here as there is a chance that new ones are instantiated during this time
// however, this is not used yet, not even sure it will be as it only serves statistics (TODO: fix)
int totalCountAfter = CurrentResources;
int difference = Math.Abs(totalCountAfter - totalCount);

LastReleased = totalCountAfter == 0 ? totalCount : difference;
TotalReleased += (ulong)LastReleased;
LastUpdated = DateTime.UtcNow;

if (State == MemoryManagerState.InProgress)
{
State = MemoryManagerState.Idle;
}

if (LastReleased > 0)
{
_logger.LogInformation("Released {0} leaking memory resources in {1}ms ({2} remains)", LastReleased, (LastUpdated - startTime).TotalMilliseconds, CurrentResources);
} else
{
Thread.Sleep(CoreConfig.MemoryManagerInterval);
}
}
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm concerned about this, not sure if this is the right way to do this?

Comment on lines +205 to +208
} catch (ThreadStateException)
{
_thread = new Thread(BackgroundThread);
_thread.Start();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this handles thread creation, if it was terminated earlier this exception is thrown

Comment on lines +180 to +181
$"Heap Memory Usage: ~{TotalMemoryMB:F5} MB ({TotalMemory} bytes)\n" +
$"Total Allocated Bytes: ~{TotalAllocatedMB:F5} MB ({TotalAllocated} bytes)");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if we should call those GC methods multiple times, I'm down for any change

_thread = new Thread(BackgroundThread);
}

[RequiresPermissions("@css/generic")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

afaik we no longer use the root flag?

@KillStr3aK KillStr3aK force-pushed the feature/memory-manager branch from ce672d3 to 01eac8b Compare October 10, 2024 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant