-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
base: main
Are you sure you want to change the base?
Conversation
<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" /> |
There was a problem hiding this comment.
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
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); | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?
} catch (ThreadStateException) | ||
{ | ||
_thread = new Thread(BackgroundThread); | ||
_thread.Start(); |
There was a problem hiding this comment.
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
$"Heap Memory Usage: ~{TotalMemoryMB:F5} MB ({TotalMemory} bytes)\n" + | ||
$"Total Allocated Bytes: ~{TotalAllocatedMB:F5} MB ({TotalAllocated} bytes)"); |
There was a problem hiding this comment.
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")] |
There was a problem hiding this comment.
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?
- VECTOR_DELETE - ANGLE_DELETE - VECTOR_COUNT - ANGLE_COUNT
ce672d3
to
01eac8b
Compare
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 stillNativeObject
,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 theIMemoryManager
that runs indefinitely (in a separate thread) by an interval that can be set in theCoreConfig
. 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 viaCoreConfig
) these will be collected, we just don't know when.Anything in the schemasystem that is
Span<T>
(#134) ORNetworkedVector<T>
(#135) (where T isVector
,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 changeCoreConfig.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:Releasing
Vector
instances:MemAlloc
Also features the
MemAlloc
class with exposed natives.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'