Fix cpu core count detection by mfkl · Pull Request #5625 · dotnet/msbuild

mfkl · 2020-08-07T17:35:34Z

Attempt to fix #435

I'm not sure whether similar code should be needed for Unix support.
I don't have a machine with > 32 cores to actually test this.

Potentially relevant:

Environment.ProcessorCount: use runtime implementation via EnvironmentAugments corefx#25630

…ount

Forgind

I think this almost works—thanks! I imagine we do care about linux, but I don't know whether the same API works. @ladipro, do you know?

Forgind · 2020-08-07T20:19:06Z

src/Shared/NativeMethodsShared.cs

+                IntPtr ptr = IntPtr.Zero;
+				try
+				{
+                    ptr = Marshal.AllocHGlobal((int)ReturnLength);


Any reason not to make this part of the original definition? If it fails, you don't have to free it.

I usually put Marshal.AllocHGlobal inside try/catch statements to catch potential OOM exceptions. Calling free with IntPtr.Zero is safe. Can change it, if you prefer.

Forgind · 2020-08-07T20:19:32Z

src/Shared/NativeMethodsShared.cs

+							{
+								count++;
+							}
+							pos += Marshal.ReadInt32(ptr, pos + 4);


nit:
Can you add this to the for statement?

Also, is this right? Maybe I'm misunderstanding, but it looks like you're increasing it by whatever 32-bit integer happens to be four bytes along? Is that the size of the current struct?

See new version.

Forgind · 2020-08-07T20:34:21Z

src/Shared/NativeMethodsShared.cs

+						int count = 0;
+						for(int pos = 0; pos < ReturnLength; )
+						{
+							LOGICAL_PROCESSOR_RELATIONSHIP Type = (LOGICAL_PROCESSOR_RELATIONSHIP)Marshal.ReadInt16(ptr, pos);


https://docs.microsoft.com/windows/win32/api/winnt/ns-winnt-system_logical_processor_information seems to say this is just a byte, not an int16.

Forgind · 2020-08-07T20:55:22Z

src/Shared/NativeMethodsShared.cs

+							LOGICAL_PROCESSOR_RELATIONSHIP Type = (LOGICAL_PROCESSOR_RELATIONSHIP)Marshal.ReadInt16(ptr, pos);
+							if(Type == LOGICAL_PROCESSOR_RELATIONSHIP.RelationProcessorCore)
+							{
+								count++;


Looking at the documentation:
The specified logical processors share a single processor core. The ProcessorCore member contains additional information.

I switched this to run whenever numberOfCpus exceeded 1 and printed out the number of cpus it calculated. It printed 6. I have 6 dual cores. I think we can get a more accurate count if we use the ProcessorCore to get processors/core as well.

Ok, reused previous corefx code, count should be more accurate now.

src/MSBuild/XMake.cs

https://github.com/dotnet/corefx/blob/8229cb7e33a40b990cdef2fd83a7a021636a89aa/src/Common/src/Interop/Windows/kernel32/Interop.GetLogicalProcessorInformationEx.cs

mfkl · 2020-08-08T13:07:24Z

I imagine we do care about linux, but I don't know whether the same API works.

Don't think so as GetLogicalProcessorInformationEx is a win32 API. There are several options for Linux/macOS, unsure which one you'd prefer.

Also, shouldn't this cpu checking code be located in https://github.com/dotnet/runtime/blob/96f178d32b7ba62485917ac46ef1edcfd3c2d10d/src/coreclr/src/classlibnative/bcltype/system.cpp#L327 instead?

ladipro

I imagine we do care about linux, but I don't know whether the same API works. @ladipro, do you know?

Isn't the issue Windows specific? I, too, wish I had a computer with >32 cores 😃

ladipro · 2020-08-10T05:16:53Z

src/Shared/NativeMethodsShared.cs

+
+        internal unsafe struct GROUP_RELATIONSHIP
+        {
+            private byte MaximumGroupCount;


nit: This is defined as

WORD MaximumGroupCount;

in the unmanaged world so should be an ushort here.

ladipro · 2020-08-10T05:25:47Z

src/Shared/NativeMethodsShared.cs

        }

+        internal enum LOGICAL_PROCESSOR_RELATIONSHIP
+		{


nit: Bad indentation.

ladipro · 2020-08-10T05:26:55Z

src/Shared/NativeMethodsShared.cs

            }
        }

+        public unsafe static int GetPhysicalCoreCount()


Can you please add a comment with a short <summary>?

ladipro · 2020-08-10T05:31:11Z

src/Shared/NativeMethodsShared.cs

+                                int groupCount = current->Group.ActiveGroupCount;
+                                for (int i = 0; i < groupCount; i++)
+                                {
+                                    processorCount += (groupInfo + i)->ActiveProcessorCount;


Add a check that this dereference is not overrunning the containing buffer, just in case.

I'm not quite sure about the kind of check you have in mind here?

byte* ptr = bufferPtr, endPtr = bufferPtr + len; while (ptr < endPtr)

This seems correct from what I can tell.
What check would you add to processorCount += (groupInfo + i)->ActiveProcessorCount;?

If groupCount is a bogus number, (groupInfo + i) could be pointing outside of the bufferPtr <-> endPtr block. I believe it's a good security practice to not trust the data and check that all memory accesses are within the bounds when using unsafe C# code.

Strictly speaking, this should also include current->Relationship and current->Group.ActiveGroupCount because ptr being less than endPtr is not sufficient for these reads to be from within the buffer.

Since this code was directly taken from dotnet/runtime unmodified, and has been pushed to production, I'm inclined to believe it works and does not need additional checks. I'll add the checks since you prefer though.

Added some checks 4223cc2, please feel free to let me know if that's not what you had in mind.

These checks will now just ignore invalid data which is, to our understanding, logically impossible. Should they not be asserting or throwing?

ladipro · 2020-08-10T05:38:06Z

src/MSBuild/XMake.cs

                                String.Equals(switchName, "maxcpucount", StringComparison.OrdinalIgnoreCase))
                            {
                                int numberOfCpus = Environment.ProcessorCount;
+                                if (numberOfCpus == 32 && NativeMethodsShared.IsWindows) // 32-bit process, 32-bit Windows had a 32-core limit


I think it would be better if either:

This code ran always, i.e. remove the numberOfCpus == 32 condition. It would help test the new code on more machines. Unless calling the Win32 API has a measurable perf impact.

Or this code ran only if the process is actually 32-bit. Users may choose to run 64-bit msbuild.exe and my understanding is that this fix is not needed there.

benvillalobos · 2020-08-10T20:34:55Z

Closing to reopen and trigger a CI build. See #5646

elachlan · 2020-08-11T05:51:44Z

src/Shared/NativeMethodsShared.cs

+
+        internal unsafe struct GROUP_RELATIONSHIP
+        {
+            private byte MaximumGroupCount;


There was a build error related to the MaximumGroupCount:
error CS0169: The field 'NativeMethodsShared.GROUP_RELATIONSHIP.MaximumGroupCount' is never used

and ActiveGroupCount:
error CS0649: Field 'NativeMethodsShared.GROUP_RELATIONSHIP.ActiveGroupCount' is never assigned to, and will always have its default value 0

and SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX.Size:
error CS0649: Field 'NativeMethodsShared.SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX.Size' is never assigned to, and will always have its default value 0

and SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX.Relationship:
error CS0649: Field 'NativeMethodsShared.SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX.Relationship' is never assigned to, and will always have its default value

Forgind

I think this works—thank you!

Forgind · 2020-08-11T20:43:24Z

src/MSBuild/XMake.cs

                                String.Equals(switchName, "maxcpucount", StringComparison.OrdinalIgnoreCase))
                            {
                                int numberOfCpus = Environment.ProcessorCount;
+                                if (IntPtr.Size == 4 && NativeMethodsShared.IsWindows) // 32-bit process, 32-bit Windows had a 32-core limit


Is there a more intuitive way to get that it's 32-bit? If not, this is fine. I was only confused until I read the comment.

If there isn't another way, can we comment with a link to https://docs.microsoft.com/en-us/dotnet/api/system.intptr.size?view=netcore-3.1, or the excerpt from that page
The size of a pointer or handle in this process, measured in bytes. The value of this property is 4 in a 32-bit process, and 8 in a 64-bit process. You can define the process type by setting the /platform switch when you compile your code with the C# and Visual Basic compilers.

To be clear, I understood immediately after reading the comment. Although I'd count this as somewhat subtle but definitely simple and functional.

Maybe extract it to a readonly static property is32bit? Or use an enum?

Good point! You reminded me of https://github.com/dotnet/msbuild/blob/master/src/Shared/EnvironmentUtilities.cs#L11

We should follow this model.

Just use this but test for the negation?

Also it looks like there is a static bool in environment.
https://github.com/dotnet/runtime/blob/ea2b09beea535abf960651c59359e40f99f2fecf/src/libraries/System.Private.CoreLib/src/System/Environment.cs#L138

https://docs.microsoft.com/en-us/dotnet/api/system.environment.is64bitprocess?view=netcore-3.1

Either way works, but I'd make a new Is32Bit version. We probably don't have to worry about 16-bit machines, and we don't currently have to worry about 128-bit machines, but just in case this code lasts a while...

EnvironmentUtilities is internal to this assembly thus cannot be referenced without adding an InternalsVisibleTo which I don't think we want to do. Used !Environment.Is64BitProcess instead.

Forgind · 2020-08-11T20:44:41Z

src/Shared/NativeMethodsShared.cs

+                        // Walk each SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX in the buffer, where the Size of each dictates how
+                        // much space it's consuming.  For each group relation, count the number of active processors in each of its group infos.
+                        int processorCount = 0;
+                        byte* ptr = bufferPtr, endPtr = bufferPtr + len;


nit:
split onto two lines

I also kinda liked your for loop better, but this is fine.

split onto two lines

done

benvillalobos · 2020-08-11T21:56:55Z

src/Shared/NativeMethodsShared.cs

+            RelationGroup,
+            RelationAll = 0xffff
+        }
+        internal struct SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX


Question for myself. I noticed this struct has more members: https://docs.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-system_logical_processor_information_ex.

Can you simply not define all members and it will populate only what you have defined without raising some sort of error?

Can you simply not define all members and it will populate only what you have defined without raising some sort of error?

Fair question. I think it depends on what you need to marshal. In case of union, this way of doing things seems to be fine and the original writer had a comment about it: https://github.com/dotnet/runtime/blob/3ab9c0b79300a5b3d954421aa4da15a9c2ca1271/src/libraries/Common/src/Interop/Windows/kernel32/Interop.GetLogicalProcessorInformationEx.cs#L27

cdmihai · 2020-08-13T22:51:31Z

src/Shared/NativeMethodsShared.cs

+        /// Useful for getting the exact core count in 32 bits processes,
+        /// as Environment.ProcessorCount has a 32-core limit in that case. 
+        /// </summary>
+        public unsafe static int GetPhysicalCoreCount()


Ideally this should be copy pasted code from a trustworthy repo (dotnet runtime, corefx, etc) and we just copy paste it and add a comment citing the source. If that's the case, can you please cite the source? (github file line permalink)

Well, ideally ideally it should be in a shared library of some sorts, but let's not get too ideal :)

Done 70d41ae

Forgind · 2020-08-16T17:29:17Z

src/Shared/NativeMethodsShared.cs

+                                if (groupInfo != null)
+                                {
+                                    int groupCount = current->Group.ActiveGroupCount;
+                                    for (int i = 0; i < groupCount; i++)


I may have misunderstood, but I thought @ladipro was thinking something more like this?

Suggested change

for (int i = 0; i < groupCount; i++)

for (int i = 0; i < groupCount && groupInfo + i < endPtr; i++)

Thanks, yes, I was thinking something like this, not the null checks added in 4223cc2. It would have to be:

for (int i = 0; i < groupCount && groupInfo + i + 1 <= endPtr; i++)

i.e. the entire structure pointed to by (groupInfo + i) fits in the buffer.

I mentioned this to @danmosemsft offline and he doesn't consider this kind of hardening necessary when calling a trusted API. @mfkl, feel free to revert the 4223cc2. Apologies for randomizing you.

Yeah, clearly we don't do it in the code this came from in dotnet/runtime and I think it would be simply "testing the OS has no bugs". But if there's a security context, don't take my word for it - we should ask others.

All good, reverted.

rainersigwald · 2020-08-17T19:23:06Z

src/MSBuild/XMake.cs

                                String.Equals(switchName, "maxcpucount", StringComparison.OrdinalIgnoreCase))
                            {
                                int numberOfCpus = Environment.ProcessorCount;
+                                if (!Environment.Is64BitProcess && NativeMethodsShared.IsWindows) // 32-bit process, 32-bit Windows had a 32-core limit


64-bit Windows had a 64-core limit, too. I think this should be called when we're running on .NET Framework regardless of bitness (on .NET Core/.NET 5, we can rely on Environment.ProcessorCount to do the trick for us, right?).

64-bit Windows had a 64-core limit, too. I think this should be called when we're running on .NET Framework regardless of bitness (on .NET Core/.NET 5, we can rely on Environment.ProcessorCount to do the trick for us, right?).

.NET 5 Environment.ProcessorCount calls the native GetProcessorCount function. So I think its all good.
https://github.com/dotnet/runtime/blob/ea2b09beea535abf960651c59359e40f99f2fecf/src/libraries/System.Private.CoreLib/src/System/Environment.cs#L13

https://github.com/dotnet/runtime/blob/60eff3f3766631bd4e7ee256ca17619fec90e9e6/src/coreclr/src/System.Private.CoreLib/src/System/Environment.CoreCLR.cs#L83-L84

https://github.com/dotnet/runtime/blob/96f178d32b7ba62485917ac46ef1edcfd3c2d10d/src/coreclr/src/classlibnative/bcltype/system.cpp#L327-L364

This reverts commit 4223cc2.

Forgind

This looks how I imagined it, thanks! I don't know whether Environment.ProcessorCount works properly on .NET Core/.NET 5+.

rainersigwald

I think this looks good to me. I'm trying to pull some strings internally to borrow a machine big enough to test it on (looks like Azure will charge me merely $50,000/month for a big enough VM!).

src/MSBuild/XMake.cs

Co-authored-by: Rainer Sigwald <raines@microsoft.com>

stackedsax · 2020-08-24T21:24:41Z

I think this looks good to me. I'm trying to pull some strings internally to borrow a machine big enough to test it on (looks like Azure will charge me merely $50,000/month for a big enough VM!).

@rainersigwald I reached out to some people within the dotnet foundation to see if there might be funds for something like this. I'll let you know if I hear anything.

elachlan · 2020-08-25T22:47:14Z

@rainersigwald I emailed AMD and they have offered to test on their 64-core ThreadRipper 3990X processor.

rainersigwald · 2020-09-03T22:31:41Z

Current status:

I got access to a Standard_F72s_v2 VM, installed a private Visual Studio with this fix and tested it out.

It almost works! In 64-bit everything looks great. In 32-bit, the new code correctly identifies 2 processor groups (2 NUMA nodes), but Windows lies to us again and claims only 32 cores in each, when really there are 36--so we're artificially limited to 64 MSBuild nodes.

This is in the docs:

In addition, the sum of all per-group ActiveProcessorCount and MaximumProcessorCount values reported in PROCESSOR_GROUP_INFO structures may exclude some active logical processors.

I'll start a thread up with some Windows folks internally to see if this is the best we can do.

Works around win32 limitation--a group will report only up to 32 processors on a 32-bit process.

rainersigwald · 2020-09-04T22:29:57Z

Latest push seems to work, at least until AMD blows things up.

rainersigwald · 2020-09-04T22:30:39Z

However, depending on how dotnet/runtime#41902 is resolved, we may need to do this on .NET Core/Windows too.

elachlan · 2020-09-04T22:46:21Z

@rainersigwald "AMD's Milan CPUs for the third generation of Epyc products are unlikely to support quadruple smt."
https://www.techpowerup.com/forums/threads/amd-zen-3-milan-cpus-not-getting-4-threads-per-core.259833/

I imagine in the future it might happen, but lucky not so soon.

rainersigwald

Tested my latest commit on the 72-core machine in dotnet build and msbuild.exe scenarios and both successfully started and used 72 nodes. Looks good to me!

src/MSBuild/XMake.cs

rainersigwald · 2020-09-18T16:41:24Z

Thank you @mfkl! This should be available starting in 16.8 preview 4.

mfkl · 2020-09-18T17:30:58Z

Thank you for picking it up and finishing this :-)

rainersigwald · 2020-09-18T18:06:23Z

By the way, GitHub somehow mangled the squash/merge commit--I did not intentionally erase you from the commit history. Sorry about that!

mfkl · 2020-09-18T18:41:27Z

No worries.

Makes three PRs (#5669, #5625, and #5653) use Change Waves (#5710) to opt out as a unit. Also added a test to ensure that change waves worked properly for one of the three PRs and corrected a minor issue with tests for changes under a change wave that assume it is enabled when there exist tests that disable the relevant change wave without resetting it to empty at the end.

dmex · 2022-09-05T17:36:32Z

@rainersigwald @mfkl

This PR for the MSBuild cpu core count detection has an issue with debugging processor groups. I've created a new issue here and included some extra information for debugging without physical hardware: #7943

Use GetLogicalProcessorInformationEx to detect correct physical CPU c…

66c9d1b

…ount

Forgind reviewed Aug 7, 2020

View reviewed changes

mfkl added 2 commits August 8, 2020 14:52

review: add missing space

e5847f6

reuse previous corefx code

8dda027

https://github.com/dotnet/corefx/blob/8229cb7e33a40b990cdef2fd83a7a021636a89aa/src/Common/src/Interop/Windows/kernel32/Interop.GetLogicalProcessorInformationEx.cs

ladipro reviewed Aug 10, 2020

View reviewed changes

benvillalobos closed this Aug 10, 2020

benvillalobos reopened this Aug 10, 2020

elachlan reviewed Aug 11, 2020

View reviewed changes

review feedback

ff27678

mfkl force-pushed the core-count-detection branch from 475840d to ff27678 Compare August 11, 2020 16:56

Forgind approved these changes Aug 11, 2020

View reviewed changes

benvillalobos reviewed Aug 11, 2020

View reviewed changes

cdmihai reviewed Aug 13, 2020

View reviewed changes

mfkl added 4 commits August 14, 2020 15:46

Use Environment.Is64BitProcess to detect 32bit process

cace766

Add original source permalink

70d41ae

split into 2 lines

2f6bfed

add checks

4223cc2

Forgind reviewed Aug 16, 2020

View reviewed changes

rainersigwald reviewed Aug 17, 2020

View reviewed changes

mfkl added 2 commits August 18, 2020 17:32

Revert "add checks"

deb5671

This reverts commit 4223cc2.

review feedback

4be5f1d

Forgind approved these changes Aug 18, 2020

View reviewed changes

rainersigwald reviewed Aug 18, 2020

View reviewed changes

src/MSBuild/XMake.cs Outdated Show resolved Hide resolved

src/MSBuild/XMake.cs Outdated Show resolved Hide resolved

mfkl and others added 2 commits August 19, 2020 13:27

review feedback - preprocessor directive

7eaa217

Co-authored-by: Rainer Sigwald <raines@microsoft.com>

review feedback - clarify comment

919beef

Co-authored-by: Rainer Sigwald <raines@microsoft.com>

Scan processor objects, not groups

538737b

Works around win32 limitation--a group will report only up to 32 processors on a 32-bit process.

rainersigwald added 2 commits September 8, 2020 10:38

Get proc-count the hard way on Windows, period

0d42e7b

Rename to Logical core count (since we do the multiplication)

30eef72

rainersigwald approved these changes Sep 14, 2020

View reviewed changes

Forgind approved these changes Sep 14, 2020

View reviewed changes

src/MSBuild/XMake.cs Outdated Show resolved Hide resolved

Always use fancy method on .NET Core

f0bb807

marcpopMSFT added the changewave16.8 label Sep 18, 2020

rainersigwald merged commit 580f1bb into dotnet:master Sep 18, 2020

mfkl deleted the core-count-detection branch September 18, 2020 17:31

rainersigwald mentioned this pull request May 10, 2022

Add Environment.TotalProcessorCount dotnet/runtime#29686

Open

dmex mentioned this pull request Sep 5, 2022

MSBuild detects wrong processor count on Windows when debugging processor groups #7943

Closed

rainersigwald mentioned this pull request Sep 12, 2022

Fix processor count on Windows for multiple processor groups #7945

Merged

rainersigwald mentioned this pull request Jun 12, 2025

Design multi-threaded build for Visual Studio scenarios #11939

Open

	for (int i = 0; i < groupCount; i++)
	for (int i = 0; i < groupCount && groupInfo + i < endPtr; i++)

Conversation

mfkl commented Aug 7, 2020 • edited by rainersigwald Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mfkl commented Aug 8, 2020

Uh oh!

ladipro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ladipro Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benvillalobos commented Aug 10, 2020

Uh oh!

elachlan Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elachlan Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfkl commented Aug 7, 2020 •

edited by rainersigwald

Loading

ladipro Aug 11, 2020 •

edited

Loading

elachlan Aug 11, 2020 •

edited

Loading

elachlan Aug 11, 2020 •

edited

Loading