Proposal: Add hard limit to MemoryInfo API

[ghost]

This is a proposal to add a new property to struct GCMemoryInfo from the coreclr repo (in src/System.Private.CoreLib/shared/System/GCMemoryInfo.cs).
The new property would be:

namespace System
{
    public readonly partial struct GCMemoryInfo
    {
        // Existing:

        public long FragmentedBytes { get; }
        public long HeapSizeBytes { get; }
        public long HighMemoryLoadThresholdBytes { get; }
        public long MemoryLoadBytes { get; }
        public long TotalAvailableMemoryBytes { get; }

        // Proposed:

        public long HardLimitBytes { get; }
    }
}

This would be the COMPLUS_GCHARDLIMIT value we got from configuration (environment variable or runtimeconfig.json);
or if a hard limit was not specified by COMPLUS_GCHARDLIMIT but the program is run in a container, this would be the hard limit calculated from the container size (currently that is 75% of the container size).
Else, this would be 0.

CC @stephentoub @janvorli @Maoni0

[Maoni0]

@davidfowl there you go

[ghost]

I noticed corefx has a file GCMemoryInfo.cs that looks identical to GCMemoryInfo.cs in coreclr -- is this the result of some automated process? I am working on a change in coreclr only right now, is it automatically copied to corefx or do I have to make the change again in corefx?

[stephentoub]

I noticed corefx has a file GCMemoryInfo.cs that looks identical to GCMemoryInfo.cs in coreclr -- is this the result of some automated process?

The https://github.com/dotnet/corefx/tree/master/src/Common/src/CoreLib directory is a mirror of https://github.com/dotnet/coreclr/tree/master/src/System.Private.CoreLib/shared; any commits that change one result in an automated PR to the other to propagate those commits. You can just make the changes in coreclr.

[stephentoub]

I am working on a change in coreclr only right now

Please work with @terrajobst to get the API reviewed before putting up a PR for it. Are you trying to get this into .NET Core 3.0? I believe with a few exceptions all new APIs for the release are supposed to be merged by tomorrow, though my information may be slightly out of date. You'll want to set the milestone on this issue appropriately, and if you believe the API is ready for review, label it as api-ready-for-review.

[davidfowl]

Else, this would be 0.

Why not make it map to whatever hard limit the GC is using? It's never actually 0 right?

[Maoni0]

@stephentoub I was the one who asked Andy to open a PR as this is changing an API that's added in 3.0 and we are merely adding a field. I was hoping we wouldn't need to go through an API review meeting for this and @terrajobst can let us know on this thread. but please let me know if I'm mistaken.

[stephentoub]

and we are merely adding a field

It's a new public property; needs to be reviewed. Even if such a review is quick.

[Maoni0]

but can it be done without us attending the API review meeting in person?

[stephentoub]

but can it be done without us attending the API review meeting in person?

It can. Things usually go more smoothly when someone in the know is there to explain it. This is @terrajobst's domain, though, so I'll let him comment. You might want to ping him on email.

[Maoni0]

@davidfowl without the hardlimit config the hard limit is just the total physical memory which is already a field in the GCMemoryInfo struct:

        /// <summary>
        /// Total available memory for the GC to use when the last GC ocurred. By default this is the physical memory on the machine, but it may be customized by specifying a HardLimit.
        /// </summary>
public long TotalAvailableMemoryBytes { get; }

[Maoni0]

just pinned @terrajobst

[davidfowl]

@davidfowl without the hardlimit config the hard limit is just the total physical memory which is already a field in the GCMemoryInfo struct:

Yea but don't make me read 2 fields. Can't we just make the value TotalAvailableMemoryBytes in the else case so i don't have to check for a 0 value?

I'm assuming this API also works when I set a percentage as well?

[Maoni0]

those are 2 different fields, TotalAvailableMemoryBytes is how much total physical memory your process is allowed to use; HardLimit is how much the GC heap is allowed to use. for some folks it's useful to know the distinction so they can decide how much to give to the GC heap vs the other memory usage.

[Maoni0]

yeh it also works when you set with percentage

[davidfowl]

those are 2 different fields, TotalAvailableMemoryBytes is how much total physical memory your process is allowed to use; HardLimit is how much the GC heap is allowed to use. for some folks it's useful to know the distinction so they can decide how much to give to the GC heap vs the other memory usage.

I'm all for having 2 values but the interesting question is whether it's useful to return 0 rather than the relevant value the GC will use in absence of the specific hard limit

[Maoni0]

0 just means GC heap is allowed to use up to TotalAvailableMemoryBytes. we could return either 0 or TotalAvailableMemoryBytes. I can go either way. no strong opinions there.

[davidfowl]

Great, I suggest we we return TotalAvailableMemoryBytes instead of 0 😄

[jlanng]

Would be nice if the presence of this config was mentioned in any consequent OOM exceptions

[bartonjs]

FXDC: LGTM (@dotnet/fxdc)

[Maoni0]

@bartonjs thanks! I presume this means it's approved

[bartonjs]

@Maoni0 I was just registering my "vote" and hoping to get a consensus swarm. It clearly didn't happen, though 😄.

[terrajobst]

Video

Looks good as proposed.

[jkotas]

The documentation for TotalAvailableMemoryBytes says:

        /// <summary>
        /// Total available memory for the GC to use when the last GC occurred. By default this is the physical memory on the machine, but it may be customized by specifying a HardLimit.
        /// </summary>
        public long TotalAvailableMemoryBytes { get; }

Is part of this proposal to change what TotalAvailableMemoryBytes means? If not, I do not understand when the values returned by TotalAvailableMemoryBytes and HardLimit are going to differ - could you please shed some light on it?

[ghost]

@jkotas Details are in the PR dotnet/corefx#25437 -- TotalAvailableMemoryBytes did not reflect a manually set hard limit before, so I didn't change what it means, just updated the documentation.

[jkotas]

I do not think we should be introducing this API. Instead, we should fix TotalAvailableMemoryBytes to return the amount of memory that the GC can play with.

I do not think it makes sense for GCMemoryInfo.TotalAvailableMemoryBytes to return total amount of memory on the machine that has nothing to do with the GC.

What are the use cases for each of GCMemoryInfo.TotalAvailableMemoryBytes and GCMemory.HardLimitBytes APIs?

[Maoni0]

this is for folks who have significant native memory usage in their managed processes where they want to have the choice to decide how much of the total memory should be given to the GC heap.

[jkotas]

This API is a getter. How do folks use this API to decide how much memory to give to the GC heap? Should this API be a setter ?

[Maoni0]

I view this API for communicating info only, not for modifying. and both values are useful to know. how do you propose we communicate both values to users?

[davidfowl]

It's also extremely useful for knowing that the set limits are effective and are being applied.

[jkotas]

I think GCMemoryInfo.TotalAvailableMemoryBytes should return how much the GC can play with. If there is a artificial hard-limit, it should return the limit. Also, I think it would be reasonable to make this limit settable.

If we need API to return various system enforced limits, I think it should be somewhere else, e.g. a type like System.Diagnostic.Process.

[davidfowl]

Sounds good to me.

[Maoni0]

that's reasonable. I'm not sure what Diagnostic.Process returns today if you are running in a container today.

as far as making this a setter - I don't imagine we'll do that for 3.0 because it would mean to define behavior for things like if you are setting a smaller limit than your current GC heap what should happen. if you feel strongly about this please open an issue and we can discuss for a future milestone.

[jkotas]

Diagnostic.Process returns today if you are running in a container today.

It returns the cgroup limit: https://github.com/dotnet/corefx/blob/1b6f45ca261467baea62ef577f8b8a7c6cf3b96c/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs#L210

[jkotas]

as far as making this a setter - I don't imagine we'll do that for 3.0

Agree. I was just trying to describe the larger context.