Allow overriding processor count via configuration setting #52492

AntonLapounov · 2021-05-08T02:19:17Z

Introduce the DOTNET_PROCESSOR_COUNT configuration settings that allows to override the number of processors available for the process. Move the GetCurrentProcessCpuCount function from GCToOSInterface to GCToEEInterface to reduce code duplication. Fixes #48094.

ghost · 2021-05-08T02:19:23Z

Tagging subscribers to this area: @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

Introduce the DOTNET_PROCESSOR_COUNT configuration settings that allows to override the number of processors available for the process. Fixes #48094.

Author:	AntonLapounov
Assignees:	-
Labels:	`area-System.Runtime`
Milestone:	-

AntonLapounov · 2021-05-08T02:22:23Z

src/coreclr/classlibnative/bcltype/system.cpp

-        // - GetCurrentProcessCpuCount() on Unixes tries to take into account cgroups CPU quota limits where applicable
-        processorCount = GetCurrentProcessCpuCount();
-    }
+    processorCount = GetCurrentProcessCpuCount();


Here and elsewhere the CanEnableThreadUseAllCpuGroups check is moved inside GetCurrentProcessCpuCount().

AntonLapounov · 2021-05-08T02:31:00Z

src/coreclr/gc/gc.cpp

-#ifndef TARGET_WINDOWS
-        // Limit the GC heaps to the number of processors available in the system.
-        nhp = min (nhp, GCToOSInterface::GetTotalProcessorCount());
-#endif // !TARGET_WINDOWS


This clipping was needed by the previous version of the code. The number of GC heaps is still limited by MAX_SUPPORTED_CPUS (always) and process_affinity_set->Count() (if affinitizing).

src/coreclr/gc/windows/gcenv.windows.cpp

AntonLapounov · 2021-05-08T02:44:49Z

src/coreclr/utilcode/util.cpp

+    {
+        m_enableGCCPUGroups = TRUE;
+        m_threadUseAllCpuGroups = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_Thread_UseAllCpuGroups) != 0;
+        m_threadAssignCpuGroups = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_Thread_AssignCpuGroups) != 0;


No functional changes in this function. Simplifying the code.

AntonLapounov · 2021-05-08T02:50:25Z

src/coreclr/utilcode/util.cpp

    if (IsInitialized())
        return;

    if (InterlockedCompareExchange(&m_initialization, 1, 0) == 0)
    {
        InitCPUGroupInfo();
-        m_initialization = -1;
+        VolatileStore(&m_initialization, -1L);


Reading/writing m_initialization without any barriers seemed like a potential race condition, so I am changing the code to use VolatileLoad/VolatileStore. There should be no performance-critical code path calling this function.

AntonLapounov · 2021-05-08T02:55:41Z

src/coreclr/utilcode/util.cpp

    return m_threadUseAllCpuGroups;
 }

 /*static*/ BOOL CPUGroupInfo::CanAssignCpuGroupsToThreads()
 {
    LIMITED_METHOD_CONTRACT;
+    _ASSERTE(m_enableGCCPUGroups || !m_threadAssignCpuGroups);


Some places in the old code used

if (CPUGroupInfo::CanEnableGCCPUGroups() && CPUGroupInfo::CanEnableThreadUseAllCpuGroups())

while other places used

if (CPUGroupInfo::CanEnableThreadUseAllCpuGroups())

Those conditions are equivalent: we set the second flag only when the first flag is set. For that reason I simplified the code everywhere and added this assert.

AntonLapounov · 2021-05-08T02:59:06Z

src/coreclr/utilcode/util.cpp

    return m_threadAssignCpuGroups;
 }
 #endif // HOST_WINDOWS

+extern SYSTEM_INFO g_SystemInfo;
+
+int GetTotalProcessorCount()


This function has been refactored from GCToOSInterface::GetTotalProcessorCount so we can use it in vm\appdomain.cpp.

AntonLapounov · 2021-05-08T03:02:14Z

src/coreclr/vm/appdomain.cpp

+    // For server GC this value indicates the number of GC heaps used in circular order to allocate sized
+    // ref handles. It must not exceed the array size allocated by the handle table (see getNumberOfSlots
+    // in objecthandle.cpp). We might want to use GetNumberOfHeaps if it were accessible here.
+    m_iNumberOfProcessors = min(GetCurrentProcessCpuCount(), GetTotalProcessorCount());


Note the comment in getNumberOfSlots:

runtime/src/coreclr/gc/objecthandle.cpp

Lines 525 to 536 in ebd695f

int getNumberOfSlots()

{

WRAPPER_NO_CONTRACT;

// when Ref_Initialize called, IGCHeap::GetNumberOfHeaps() is still 0, so use #procs as a workaround

// it is legal since even if later #heaps < #procs we create handles by thread home heap

// and just have extra unused slots in HandleTableBuckets, which does not take a lot of space

if (!IsServerHeap())

return 1;

return GCToOSInterface::GetTotalProcessorCount();

}

AntonLapounov · 2021-05-10T05:11:41Z

I am puzzled why I am hitting this linker error on macOS:

[ 93%] Linking CXX shared library libmscordbi.dylib
Undefined symbols for architecture x86_64:
  "_PAL_GetTotalCpuCount", referenced from:
      GetTotalProcessorCount() in libutilcodestaticnohost.a(util.cpp.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

The util.cpp file uses PAL_GetLogicalCpuCountFromOS and I added usage of PAL_GetTotalCpuCount, which is defined in the same src\coreclr\pal\src\misc\sysinfo.cpp file. Somehow the linker is OK with the former, but not with the latter. I do not have a macOS device and lab build logs seem useless. On Ubuntu it links just fine and I do not see any macOS-specific #ifdefs in the code. Is there a way to debug this?

janvorli · 2021-05-10T15:52:10Z

I am puzzled why I am hitting this linker error on macOS:

The reason is that mscordbi doesn't use PAL, but relies on symbols exported by mscordaccore instead. So you'll need to add the PAL_GetTotalCpuCount to the exported symbols of mscordaccore (in https://github.com/dotnet/runtime/blob/main/src/coreclr/dlls/mscordac/mscordac_unixexports.src)

jkotas · 2021-05-10T15:57:45Z

cc @dotnet/gc for the GC related changes.

src/coreclr/gc/gcconfig.h

AntonLapounov · 2021-05-11T17:15:23Z

Any additional feedback?

kouvel · 2021-05-11T17:23:43Z

I'll take a look as well

mangod9 · 2021-05-11T18:23:30Z

Do you plan to add some tests to validate this, or will that be a separate PR?

AntonLapounov · 2021-05-11T19:14:31Z

There will be another PR to respect the job CPU limit. I can include a test that will test both settings.

janvorli

LGTM, thank you!

kouvel

LGTM, thanks!

AntonLapounov · 2021-05-11T22:33:49Z

Thank you all for the feedback and great suggestion that allowed to reduce code duplication.

Allow overriding processor count via configuration setting

5cf327a

dotnet-issue-labeler bot added the area-System.Runtime label May 8, 2021

AntonLapounov commented May 8, 2021

View reviewed changes

src/coreclr/gc/windows/gcenv.windows.cpp Outdated Show resolved Hide resolved

AntonLapounov commented May 8, 2021

View reviewed changes

Fix Linux/macOS build

cc1c6cd

AntonLapounov force-pushed the OverrideProcessorCount branch from 1add54d to cc1c6cd Compare May 8, 2021 03:46

AntonLapounov requested review from janvorli and jkotas May 10, 2021 05:09

jkotas requested a review from kouvel May 10, 2021 15:54

Fix macOS build

5644fc9

Maoni0 reviewed May 10, 2021

View reviewed changes

src/coreclr/gc/gcconfig.h Outdated Show resolved Hide resolved

AntonLapounov added 5 commits May 10, 2021 17:08

Move GetCurrentProcessCpuCount function to GCToEEInterface

f7673e4

Remove more dead code

b713b33

Merge branch 'main' into OverrideProcessorCount

bfb2173

Empty commit

96e3ad3

Revert random change

7dd6a81

janvorli approved these changes May 11, 2021

View reviewed changes

kouvel approved these changes May 11, 2021

View reviewed changes

AntonLapounov merged commit 28ec201 into dotnet:main May 11, 2021

AntonLapounov deleted the OverrideProcessorCount branch May 11, 2021 22:32

karelz added this to the 6.0.0 milestone May 20, 2021

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow overriding processor count via configuration setting #52492

Allow overriding processor count via configuration setting #52492

AntonLapounov commented May 8, 2021 •

edited

Loading

ghost commented May 8, 2021

AntonLapounov May 8, 2021

AntonLapounov May 8, 2021

AntonLapounov May 8, 2021

AntonLapounov May 8, 2021

AntonLapounov May 8, 2021

AntonLapounov May 8, 2021

AntonLapounov May 8, 2021

AntonLapounov commented May 10, 2021

janvorli commented May 10, 2021

jkotas commented May 10, 2021

AntonLapounov commented May 11, 2021

kouvel commented May 11, 2021

mangod9 commented May 11, 2021

AntonLapounov commented May 11, 2021

janvorli left a comment

kouvel left a comment

AntonLapounov commented May 11, 2021

	int getNumberOfSlots()
	{
	WRAPPER_NO_CONTRACT;

	// when Ref_Initialize called, IGCHeap::GetNumberOfHeaps() is still 0, so use #procs as a workaround
	// it is legal since even if later #heaps < #procs we create handles by thread home heap
	// and just have extra unused slots in HandleTableBuckets, which does not take a lot of space
	if (!IsServerHeap())
	return 1;

	return GCToOSInterface::GetTotalProcessorCount();
	}

Allow overriding processor count via configuration setting #52492

Allow overriding processor count via configuration setting #52492

Conversation

AntonLapounov commented May 8, 2021 • edited Loading

ghost commented May 8, 2021

AntonLapounov May 8, 2021

Choose a reason for hiding this comment

AntonLapounov May 8, 2021

Choose a reason for hiding this comment

AntonLapounov May 8, 2021

Choose a reason for hiding this comment

AntonLapounov May 8, 2021

Choose a reason for hiding this comment

AntonLapounov May 8, 2021

Choose a reason for hiding this comment

AntonLapounov May 8, 2021

Choose a reason for hiding this comment

AntonLapounov May 8, 2021

Choose a reason for hiding this comment

AntonLapounov commented May 10, 2021

janvorli commented May 10, 2021

jkotas commented May 10, 2021

AntonLapounov commented May 11, 2021

kouvel commented May 11, 2021

mangod9 commented May 11, 2021

AntonLapounov commented May 11, 2021

janvorli left a comment

Choose a reason for hiding this comment

kouvel left a comment

Choose a reason for hiding this comment

AntonLapounov commented May 11, 2021

AntonLapounov commented May 8, 2021 •

edited

Loading