Perform PhysicalMemoryLimit check for workstation GC, refactor GetLargestOnDieCacheSize into GetCacheSizePerLogicalCpu #15975
Conversation
| @@ -1854,9 +1854,10 @@ DWORD GetLogicalCpuCountFallback() | |||
|
|
|||
| #endif // _TARGET_X86_ || _TARGET_AMD64_ | |||
|
|
|||
| size_t GetLargestOnDieCacheSize(BOOL bTrueSize) | |||
| // fix this if/when AMD does multicore or SMT | |||
There was a problem hiding this comment.
I guess this is a little outdated now... 😆
| @@ -1911,6 +1912,31 @@ size_t GetLargestOnDieCacheSize(BOOL bTrueSize) | |||
| } | |||
| } | |||
|
|
|||
| // TODO: Currently GetLogicalCpuCountFromOS() and GetLogicalCpuCountFallback() are broken on | |||
There was a problem hiding this comment.
This whole codepath seems to be really outdated to me. At one point I asked around to see if these comments still apply, and to the knowledge of everyone I talked to the OS issues that this function is working around were fixed a long time ago.
We should probably rewrite this whole thing at some point...
There was a problem hiding this comment.
We should be using GetLogicalProcessorInformationEx on the OS versions where it's available and fall back to cpuid where it's not. but you don't have to do it in this PR; feel free to open an issue to track if you don't want to do it in this PR.
There was a problem hiding this comment.
I have created https://github.com/dotnet/coreclr/issues/16071.
|
|
||
| int n_heaps = gc_heap::n_heaps; | ||
| #else //SERVER_GC | ||
| size_t trueSize = GCToOSInterface::GetCacheSizePerLogicalCpu(TRUE); |
There was a problem hiding this comment.
@Maoni0 @swgillespie I wonder, for workstation gc, if we want to multiply here with GCToOSInterface::GetCurrentProcessCpuCount. wdyt?
There was a problem hiding this comment.
I'll leave that to @Maoni0, I don't have any thoughts - not super familiar with this math.
There was a problem hiding this comment.
we don't want to do that 'cause we don't assume that processes using workstation GC will own the machine. unless you specifically affinitize your process with fewer CPUs, GetCurrentProcessCpuCount will return all CPUs.
|
@swgillespie @Maoni0 thanks for reviewing! Can one of you merge this PR? |
|
thanks for doing this, @tmds! |
Some time ago I was looking into gc.cpp implementation of checking physical memory: https://github.com/dotnet/coreclr/issues/14991#issuecomment-348428003
This PR is making two changes:
CC @swgillespie @Maoni0