Runtime: OutOfMemory: NativeHeapMemoryBlock should call GC.AddMemoryPressure.

[GPSnoopy]

I believe that calling Marshal.AllocHGlobal() without calling GC.AddMemoryPressure (and subsequently GC.RemoveMemoryPressure when releasing the memory) is a mistake.

https://github.com/dotnet/runtime/blob/4f9ae42d861fcb4be2fcd5d3d55d5f227d30e723/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/NativeHeapMemoryBlock.cs

See https://github.com/dotnet/roslyn/issues/24939 for a strong example of why we care about this and why it can lead to OutOfMemory exceptions.

[jkotas]

There are number of cases where memory allocated by Marshal.AllocHGlobal is de-allocated by unmanaged code that the .NET runtime has no control over; and vice versa. Adding GC pressure calls to Marshal.AllocHGlobal would lead to incorrect accounting of GC pressure in these fairly common cases.

[sharwell]

@jkotas note that this issue specifically relates to System.Reflection.Metadata's use of AllocHGlobal. If these cases are not getting de-allocated by native code then we should fix them.

[jkotas]

Ok, I got confused by this being marked with area-GC

[Gnbrkm41]

Is it something worth reviewing, to see if codes that allocate in unmanaged memory and de-allocate call AddMemoryPressure / RemoveMemoryPressure?

When working with dotnet/corefx#42472 I noticed that this code uses AllocHGlobal and frees the memory once finished but it seems like there isn't any calls to AddMemoryPressure and RemoveMemoryPressure.

[GPSnoopy]

@Gnbrkm41 IMHO Most likely yes. I would wager this is a common mistake.

[jkotas]

It is not a mistake. 99.99+% code that allocates native memory does not call AddMemoryPressure / RemoveMemoryPressure and it is just fine.

The GC does query the overall state of the process and runs more aggressively when it sees that the memory is getting tight.

Also, the best practice is to use IDisposable to explicitly dispose expensive unmanaged resources that includes large unmanaged memory blocks. AddMemoryPressure / RemoveMemoryPressure does not help much in this case.

AddMemoryPressure / RemoveMemoryPressure helps for cases where the unmanaged resources are disposed via finalizers. It was originally introduced for built-in COM interop that typically does this.

[GPSnoopy]

@jkotas Are you saying that the problem in the initial ticket is that Roslyn does not explicitly dispose of its PEReader/NativeHeapMemoryBlock instances?

Looking at MetadataReference.CreateFromFile() and similar methods in Roslyn, they do not expose a Dispose method. In fact the follow doc says this:
https://docs.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.metadatareference.createfromfile?view=roslyn-dotnet#Microsoft_CodeAnalysis_MetadataReference_CreateFromFile_System_String_Microsoft_CodeAnalysis_MetadataReferenceProperties_Microsoft_CodeAnalysis_DocumentationProvider_

The method eagerly reads the entire content of the file into native heap. The native memory block is released when the resulting reference becomes unreachable and GC collects it. To decrease memory footprint of the reference and/or manage the lifetime deterministically use CreateFromFile(String) to create an IDisposable metadata object and GetReference(DocumentationProvider, ImmutableArray, Boolean, String, String) to get a reference to it.

[jkotas]

Right, depending on the finalizers to free big chunks of unmanaged memory is a recipe for problems.

[bencyoung]

Agreed that you shouldn't unconditionally call AddMemoryPressure. We run processes with typically 10-20GB of managed memory, and 100s of GB of unmanaged. When we used to use AddMemoryPressure when allocating the unmanaged all it did was cause the GC to thrash as it had a misconceived notion of how much memory it had any influence over!

[tannergooding]

Also, the best practice is to use IDisposable to explicitly dispose expensive unmanaged resources that includes large unmanaged memory blocks. AddMemoryPressure / RemoveMemoryPressure does not help much in this case.

@jkotas, I thought for large allocations you wanted both? That is you want IDisposable to ensure the resource is freed properly without relying on the finalizer, but that won't inform the GC that you allocated 3GB of native memory while that memory lives (especially if it is long lived). So the pattern would be essentially:

SomeConstructor()
{
    _handle = AllocateMemory(3GB);
    AddMemoryPressure(3GB);
}

~Finalizer()
{
    Dispose(isDisposing: false);
}

Dispose()
{
    Dispose(isDisposing: true);
    GC.SuppressFinalize();
}

Dispose(bool isDisposing)
{
    FreeMemory(_handle);
    RemoveMemoryPressure(3GB);
}

If that isn't the case, maybe the documentation (https://docs.microsoft.com/en-us/dotnet/api/system.gc.addmemorypressure?view=netframework-4.8) could be improved to better detail when you might want to use the API and provide an example of how to use it properly. CC. @Maoni0

[jkotas]

If it helps, the implementation of the Add/RemoveMemoryPressure API is here: https://github.com/dotnet/runtime/blob/c0ddd1c5d1636de873398c8d9544e02289f95fec/src/coreclr/src/vm/comutilnative.cpp#L1282

Notice that it is not actually built into the GC. It is a simple control loop with a few statics: Cumulative counter of the total memory pressure, and a threshold.

• When the cumulative counter goes over the threshold, we compute a new bigger threshold and potentially trigger a GC. It is throttled to avoid triggering the GCs too often.
• When the cumulative counter goes under 1/4 of the threshold, we compute a new smaller threshold.

I agree that the docs for this can be better.

[tannergooding]

Thanks for the reference! That's definitely not what I would have expected given the remarks section of the APIs 😄

[ericstj]

So what's the concensus here: just update the docs for GC.Add/RemoveMemoryPressure or should we be adding calls to S.R.M? cc @tmat

[jkotas]

I have submitted doc update as https://github.com/dotnet/dotnet-api-docs/pull/4812