-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify numasupport #84207
Simplify numasupport #84207
Conversation
Tagging subscribers to this area: @hoyosjs Issue DetailsAimed to remove lib dependency.
|
de0ae6a
to
4a2a0ff
Compare
118e72b
to
420c0b8
Compare
Also delete libnuma mentions from: |
29e60e1
to
af2144e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! @janvorli ?
Do we have any NUMA machines to verify that this works well? cc @dotnet/gc
My main Linux devbox has NUMA enabled, so I can test it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo the comment. I've verified on my NUMA enabled physical device that it works correctly.
src/coreclr/gc/unix/numasupport.cpp
Outdated
return; | ||
|
||
g_numaAvailable = true; | ||
g_highestNumaNode = GetNodeNum("/sys/devices/system/node", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original (dynamic) code was disabling NUMA if there was just one NUMA node. In that case, it is useless to add complexities related to NUMA. I think we should do it here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
CI failures are known (according to Build Analysis). cc @jkotas, I'll update SDK tests (which counts the number of assets in |
Aimed to remove lib dependency.