Add property HardLimitBytes to GCMemoryInfo #25437

ghost · 2019-06-26T17:16:07Z

This adds a new property HardLimitBytes.

Unlike TotalAvailableMemoryBytes,
this will reflect an explicitly set COMPLUS_GCHeapHardLimit.

It will also reflect the fraction of a container's size that we use,
where TotalAvailableMemoryBytes is the total container size.

Normally, though, it is equal to TotalAvailableMemoryBytes.

Fix dotnet/corefx#38821

This adds a new property HardLimitBytes. Unlike TotalAvailableMemoryBytes, this will reflect an explicitly set COMPLUS_GCHeapHardLimit. It will also reflect the fraction of a container's size that we use, where TotalAvailableMemoryBytes is the total container size. Normally, though, it is equal to TotalAvailableMemoryBytes. Fix #38821

ghost · 2019-06-26T17:17:57Z

I've tested this out and here are the results (my notes in parentheses):

Running normally:

bin/Debug/netcoreapp3.0/win-x64/publish/cs.exe
TotalAvailableMemoryBytes: 137323081728
HardLimitBytes: 137323081728 (normally equal)

Running in a container:

C:/Users/anhans/work/dotnet-gc-infra/c_scripts/run_in_job.exe --memory-mb 123 -- bin/Debug/netcoreapp3.0/win-x64/publish/cs.exe
TotalAvailableMemoryBytes: 128974848 (full container size)
HardLimitBytes: 96731136 (75% of container size)

Running with a hard limit :

$env:COMPLUS_GCHeapHardLimit="75bcd15" # value is hexadecimal for 123456789
bin/Debug/netcoreapp3.0/win-x64/publish/cs.exe
TotalAvailableMemoryBytes: 137323081728 (the machine's physical memory)
HardLimitBytes: 123456789 (what I specified)

jkotas · 2019-06-26T18:19:20Z

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.

sergiy-k · 2019-06-27T00:57:59Z

@Maoni0, @andy-ms, I agree with @jkotas. Why TotalAvailableMemoryBytes cannot be fixed to return the Hard Limit value if it has been specified? Is there a technical reason for that?

…avior

ghost · 2019-06-28T01:05:29Z

Added a new commit which makes TotalAvailableMemoryBytes have the behavior that HardLimitBytes had.

src/System.Private.CoreLib/shared/System/GCMemoryInfo.cs

jkotas

LGTM modulo nit.

jkotas

Actually, I have just notice that this value is used to convert highMemLoadThreshold and lastRecordedMemLoad to bytes. Is this conversion still valid?

ghost · 2019-07-02T02:39:59Z

You're right, in GCToOSInterface::GetMemoryStatus (called by get_memory_info which is what we use to get the memory load):

uint64_t restricted_limit = GetRestrictedPhysicalMemoryLimit();
...
*memory_load = (uint32_t)((float)workingSetSize * 100.0 / (float)restricted_limit);

GetRestrictedPhysicalMemoryLimit() appears to handle containers but not an explicit hard limit. So with an explicit hard limit set, memory_load_th basically has no effect as it will likely be bigger than the hard limit. This may be an oversight, but better to keep these numbers accurate, so I'll return both physical and hard limits from c++ and have c# choose between them.

jkotas · 2019-07-02T02:43:23Z

I think it would be best to return the numbers in bytes from the GC API, so there are no further adjustments needed in C#.

Maoni0 · 2019-07-02T02:57:54Z

@andy-ms

GetRestrictedPhysicalMemoryLimit() appears to handle containers but not an explicit hard limit.

GetRestrictedPhysicalMemoryLimit() is not supposed to read what you set with the hardlimit configs. it's supposed to return the true physical memory limit for the process.

davidfowl · 2019-07-02T03:07:40Z

I think it would be best to return the numbers in bytes from the GC API, so there are no further adjustments needed in C#.

+1

so we can compute highMemoryLoadThresholdBytes and memoryLoadBytes

jkotas · 2019-07-02T05:16:42Z

src/gc/gc.cpp

+    *highMemLoadThresholdBytes = (uint64_t) (((double)gc_heap::high_memory_load_th) / 100 * gc_heap::total_physical_mem);
+    *totalAvailableMemoryBytes = gc_heap::heap_hard_limit != 0 ? gc_heap::heap_hard_limit : gc_heap::total_physical_mem;
+    *lastRecordedMemLoadBytes = (uint64_t) (((double)gc_heap::last_gc_memory_load) / 100 * gc_heap::total_physical_mem);
+    *lastRecordedMemLoadPct = gc_heap::last_gc_memory_load;


Is the last_gc_memory_load relative to total_physical_mem; or relative to *totalAvailableMemoryBytes ?

Either this or the previous line looks wrong. When memory load is 100%, I think loadBytes should be equal totalAvailableMemoryBytes.

It may be best to get rid of the *lastRecordedMemLoadPct argument, and just divide lastRecordedMemLoadBytes and totalAvailableMemoryBytes on the CoreLib side.

last_gc_memory_load is relative to total_physical_mem. the problem is we do have a GetMemoryLoad that needs the actual memory load.

I would really prefer our original API proposal that had both the total physical memory and the hardlimit for the GC heap 'cause we do care about both. and I don't think it's unreasonable for the users to know about both. either that or we have another API (internal) that gets the actual physical memory load.

last_gc_memory_load is relative to total_physical_mem. If totalAvailableMemoryBytes is less than total_physical_mem, then it will be impossible for last_gc_memory_load to ever reach 100% -- it would be at most totalAvailableMemoryBytes / total_physical_mem * 100.

The primary reason why we have introduced the GCMemoryInfo API is to allow folks to build self-tuning caches.

Could you please review the places where this API is used currently (our TlsOverPerCoreLockedStacksArrayPool in particular - not sure whether there is anything else) and validate that it works correctly in all cases (regular VMs, containers, hardlimit)?

The current code for this is here:

coreclr/src/System.Private.CoreLib/shared/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.cs

Line 303 in 4ca032d

GCMemoryInfo memoryInfo = GC.GetGCMemoryInfo();

. Is this code correct for all situations? If not, what is the code that we want people to write to implement this logic? Once we know what code we want people to write here, we should be clear about what these APIs needs to do.

GetMemoryLoad that needs the actual memory load.

This is used by the internal experimental MemoryLoadChangeNotification APIs. Is this API used by anything?

the impl in TlsOverPerCoreLockedStacksArrayPool.cs looks correct - it could be improved (for example it could say only trim if the size it trims could actually make a difference in memory pressure) but I don't think we are doing that for 3.0.

it seems awkward to have to fit the 2 numbers (actual physical memory and hardlimit) into 1. and if we don't want to have to call the diagnostics API to retrieve the physical memory (which also seems awkward for GC) our original proposal seems more sensible to me.

What logic do we think would need to look at physical memory rather than the hard limit?

We have received a lot of feedback in 2.x about ArrayPool keeping too much memory over time. We have seen people taking extreme measures like poking the ArrayPool via private reflection to make it release the memory. We have implemented the trimming at the last minute to mitigate the situation.

If the ArrayPool trimming does not take the hardlimit into account, I expect that we are going to hear the same feedback again from folks who try to use the hardlimit.

Right I'm agreeing with your assessment. I'm mostly wondering when you'd need the actual physical memory vs the hard limit (the other should probably be exposed where we have the other process specific APIs Process.*)

(My reply was for Maoni's comment.)

I agree that the actual physical memory does not seem to be necessary to build decent trimming heuristic.

I think that there are 4 numbers to feed into a decent basic trimming heuristic:

The GC heap size: current value and limit that it should stay under. I believe it is HeapSizeBytes and TotalAvailableMemoryBytes in the current API.

The memory load (ie the GC heap size plus everything else): current value and limit that it should stay under. I believe it is MemoryLoadBytes and HighMemoryLoadThresholdBytes in the current API.

When either of these gets too close to its limit, it is a signal to trim caches.

I have opened https://github.com/dotnet/coreclr/issues/25552 on ArrayPool not respecting GCHardLimit.

jkotas · 2019-07-02T05:21:13Z

src/gc/gcinterface.h

+    virtual void GetMemoryInfo(uint64_t* highMemLoadThresholdBytes,
+                               uint64_t* totalPhysicalMemoryBytes,
+                               uint64_t* lastRecordedMemLoadBytes,
+                               uint32_t* lastRecordedMemLoadPct,
                               size_t* lastRecordedHeapSize,


Nit: Add Bytes suffix to these two args; or delete it from the other args here.

jkotas

As I have said, it would be nice to get rid of the lastRecordedMemLoadPct because of it is redudant piece of data and change MemoryLoadChangeNotification to use the public GCMemoryInfo properties. I think it should be using MemoryLoadBytes / HighPressureThreshold just like ArrayPool.

The MemoryLoadChangeNotification is internal experimental API that is not used by anything. As far as I can tell, it is broken current and nobody can use it for anything useful. So passing lastRecordedMemLoadPct through may be confusing, but it is not actively harmful.

The rest of the changes looks like an improvement to me, so I would be ok with merging this (either as is; or with cleaned up lastRecordedMemLoadPct).

…ange

jkotas · 2019-07-08T23:13:16Z

Thank you everyone!

MeiChin-Tsai · 2019-07-08T23:23:13Z

Yes, I like the change. It is good to make intension explicit.

Suchiman · 2019-07-09T08:23:44Z

src/gc/gc.cpp

+                           size_t* lastRecordedFragmentationBytes)
+{
+    *highMemLoadThresholdBytes = (uint64_t) (((double)gc_heap::high_memory_load_th) / 100 * gc_heap::total_physical_mem);
+    *totalAvailableMemoryBytes = gc_heap::heap_hard_limit != 0 ? gc_heap::heap_hard_limit : gc_heap::total_physical_mem;


Is it fine to potentially return heap_hard_limit here, which could be larger than total_physical_mem if inproperly configured?

If you misconfigure the hard limit, the GC itself won't work quite right either. You get what you asked for. We are not trying to guess whether you meant it or not.

…tnet/coreclr#25437) * Add property HardLimitBytes to GCMemoryInfo This adds a new property HardLimitBytes. Unlike TotalAvailableMemoryBytes, this will reflect an explicitly set COMPLUS_GCHeapHardLimit. It will also reflect the fraction of a container's size that we use, where TotalAvailableMemoryBytes is the total container size. Normally, though, it is equal to TotalAvailableMemoryBytes. Fix dotnet/coreclr#38821 * Remove HardLimitBytes; have TotalAvailableMemoryBytes take on its behavior * Fix typos * Separate total_physical_mem and heap_hard_limit so we can compute highMemoryLoadThresholdBytes and memoryLoadBytes * Do more work in gc.cpp instead of Gc.cs * Consistently end names in "Bytes" Commit migrated from dotnet/coreclr@11137fb

Remove HardLimitBytes; have TotalAvailableMemoryBytes take on its beh…

b095f24

…avior

jkotas reviewed Jun 28, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/GCMemoryInfo.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 28, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/GCMemoryInfo.cs Outdated Show resolved Hide resolved

Fix typos

a9c48ae

jkotas reviewed Jun 28, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/GCMemoryInfo.cs Show resolved Hide resolved

jkotas approved these changes Jun 28, 2019

View reviewed changes

jkotas suggested changes Jun 28, 2019

View reviewed changes

Andy Hanson added 2 commits July 1, 2019 20:16

Separate total_physical_mem and heap_hard_limit

6447db8

so we can compute highMemoryLoadThresholdBytes and memoryLoadBytes

Do more work in gc.cpp instead of Gc.cs

c485038

jkotas reviewed Jul 2, 2019

View reviewed changes

Consistently end names in "Bytes"

cb8f824

jkotas approved these changes Jul 3, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into just_hardlimit_ch…

a92324c

…ange

Maoni0 approved these changes Jul 8, 2019

View reviewed changes

jkotas merged commit 11137fb into dotnet:master Jul 8, 2019

ghost deleted the just_hardlimit_change branch July 8, 2019 23:14

ghost requested a review from MeiChin-Tsai July 8, 2019 23:14

Suchiman reviewed Jul 9, 2019

View reviewed changes

jkotas mentioned this pull request Dec 13, 2019

ArrayPool trimming does not respect GCHardLimit dotnet/runtime#853

Open

StephenMolloy mentioned this pull request Feb 17, 2021

Add PhysicalMemoryLimit capabilities for /proc/meminfo OS's like Linux. dotnet/runtime#47783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add property HardLimitBytes to GCMemoryInfo #25437

Add property HardLimitBytes to GCMemoryInfo #25437

ghost commented Jun 26, 2019 •

edited by ghost

Loading

ghost commented Jun 26, 2019

jkotas commented Jun 26, 2019

sergiy-k commented Jun 27, 2019

ghost commented Jun 28, 2019

jkotas left a comment

jkotas left a comment

ghost commented Jul 2, 2019

jkotas commented Jul 2, 2019

Maoni0 commented Jul 2, 2019

davidfowl commented Jul 2, 2019

jkotas Jul 2, 2019 •

edited

Loading

Maoni0 Jul 2, 2019

ghost Jul 2, 2019

jkotas Jul 2, 2019 •

edited

Loading

Maoni0 Jul 3, 2019

davidfowl Jul 3, 2019

jkotas Jul 3, 2019 •

edited

Loading

davidfowl Jul 3, 2019

jkotas Jul 3, 2019

jkotas Jul 3, 2019

jkotas Jul 2, 2019

jkotas left a comment •

edited

Loading

jkotas commented Jul 8, 2019

MeiChin-Tsai commented Jul 8, 2019

Suchiman Jul 9, 2019

jkotas Jul 9, 2019

Add property HardLimitBytes to GCMemoryInfo #25437

Add property HardLimitBytes to GCMemoryInfo #25437

Conversation

ghost commented Jun 26, 2019 • edited by ghost Loading

ghost commented Jun 26, 2019

jkotas commented Jun 26, 2019

sergiy-k commented Jun 27, 2019

ghost commented Jun 28, 2019

jkotas left a comment

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

ghost commented Jul 2, 2019

jkotas commented Jul 2, 2019

Maoni0 commented Jul 2, 2019

davidfowl commented Jul 2, 2019

jkotas Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas left a comment • edited Loading

Choose a reason for hiding this comment

jkotas commented Jul 8, 2019

MeiChin-Tsai commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 26, 2019 •

edited by ghost

Loading

jkotas Jul 2, 2019 •

edited

Loading

jkotas Jul 2, 2019 •

edited

Loading

jkotas Jul 3, 2019 •

edited

Loading

jkotas left a comment •

edited

Loading