Skip to content
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

Fixed get count threads for multiprocessor system with NUMA architecture #1514

Closed
wants to merge 1 commit into from

Conversation

GermanAizek
Copy link
Contributor

@GermanAizek GermanAizek commented Nov 10, 2023

Fixed very old problem that only on any Windows NT and modern Windows Server 😆

https://developercommunity.visualstudio.com/t/hardware-concurrency-returns-an-incorrect-result/350854

https://stackoverflow.com/questions/31209256/reliable-way-to-programmatically-get-the-number-of-hardware-threads-on-windows

Why this commit is useful not only for server configurations, now a very cheap PC configuration is building from Xeon E54xx, X34xx, E3-xxxx, E5-xxxx, E7-xxx, any Silver, any Gold, any Platinum series, cheapest on LGA 2011v3 socket two-socket board with NUMA support is cheap on Aliexpress, Baidu or Alibaba.

https://aliexpress.ru/item/1005004510711777.html

@Xottab-DUTY
Copy link
Member

Xottab-DUTY commented Nov 10, 2023

Do you have NUMA yourself and have any problems with OpenXRay?
Oh, this is actually a pull request, not an issue, sorry :D

@@ -213,12 +213,63 @@ void CalcIterations()
ttapi_dwFastIter = u32((iterations * frequency) / ((end - start) * 50000));
}

// Lainon: This implementation supports both conventional single-cpu PC configurations
// and multi-cpu system on NUMA architecture
size_t NUMAHardwareConcurrency() noexcept
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should be defined in _math.h and _math.cpp instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix it 7218c71

@@ -236,7 +237,11 @@ void _initialize_cpu()
listFeature("AltiVec", SDL_HasAltiVec());

Msg("* CPU features: %s", features);
#if defined(XR_PLATFORM_WINDOWS)
Msg("* CPU threads: %d", NUMAHardwareConcurrency());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to just name this function something like GetThreadsCounts and place it into CPU namespace in _math.h and use it instead of std::thread::hardware_concurrency()); – that means we also should create a Linux, etc. version of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix it 7218c71

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish people were saying "physical/logical cores", not "threads". It's so confusing and grows people illiterate IMHO.

@Hrusteckiy
Copy link
Contributor

numa numa numa ei

@GermanAizek GermanAizek changed the title Added NUMA architecture support for multiprocessig system Fixed get count threads for multiprocessor system with NUMA architecture Nov 13, 2023
Comment on lines 211 to 214
if (concurrency == 0)
return std::thread::hardware_concurrency();
else
return concurrency;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (concurrency == 0)
return std::thread::hardware_concurrency();
else
return concurrency;
return concurrency == 0 ? std::thread::hardware_concurrency() : concurrency;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix it 7dc0960

@GermanAizek
Copy link
Contributor Author

GermanAizek commented Dec 8, 2023

@Xottab-DUTY you can do a code review

steamuserimages-a akamaihd

@GermanAizek
Copy link
Contributor Author

@Xottab-DUTY,
More optimized variant detection NUMA and return count threads on host, must added before GetLogicalProcessorInformationEx:

    if (!IsNUMA())
        return single_cpu_concurrency();

    if (GetLogicalProcessorInformationEx(RelationAll, nullptr, &length) != FALSE)
    {
        return single_cpu_concurrency();
    }
bool IsNUMA() noexcept
{
    ULONG HighestNodeNumber;
    return !(!GetNumaHighestNodeNumber(&HighestNodeNumber) || HighestNodeNumber == 0);
}

@Xottab-DUTY
Copy link
Member

Platform-specific code is generally unwanted if we can use standard C++ functionality and/or SDL.
I'm not sure if C++ and SDL has the functionality proposed in this PR, so this PR may actually be useful, but I'm closing this PR along with banning (second time) it's author due to his disrespect to repository maintainers (almost all his PRs were submitted in broken state and/or with questionable changes, while rejecting to fix his own PRs, along with pinging me personally without any reason).

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants