Currently for windows only, this is an initial commit towards native
support of NUMA.
Current commit makes it so Cycles will use all logical processors on
Windows running on system with more than 64 threads.
Details
Details
- Reviewers
Mai Lavelle (maiself) Brecht Van Lommel (brecht) Thomas Dinges (dingto) Lukas Stockner (lukasstockner97) Martijn Berger (juicyfruit) - Commits
- rBSb62faa54de70: Cycles: Add support of processor groups
rB95c663791ce1: Cycles: Add support of processor groups
rCb160722f3394: Add support of processor groups
rBb62faa54de70: Cycles: Add support of processor groups
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- arcpatch-D2049
Event Timeline
Comment Actions
Couple of notes:
- This bumps up requirement to WIndows7+, but perhaps we can solve that b doing dlopen() type of thing.
- Seems it doesn't work really well on a systems with uneven separation of threads between groups, need to doublecheck that.
| intern/cycles/util/util_thread.cpp | ||
|---|---|---|
| 49 | this overflows when you go over 31 threads per group. 1ull works, but overflows at 64 threads per group. group_affinity.Mask = (num_threads == 64) ? -1 : (1ull << num_threads) - 1; | |
Comment Actions
I'd ideally see this patch extended to offer the option of setting a render threads affinity to a specific core in a group (with the idea being, if a thread runs on the same core , it can re-use the l1/l2 caches on the cpu), I ran some tests on an i7-3770 recently and got some small but measurable gains. (Regular being a master build, TA same build but with each render thread assigned a specific core)
bmw 27 cpu Regular 17:00.12 1020.12s 100% TA 16.33.25 993.25s 97.4% Koro Regular 34.52.51 2092.51s 100% TA 32:37.37 1957.37s 93.5% Fishy_cat Regular 24:29:23 1469.23s 100% TA 24:03.96 1443.96s 98.3%
could be interesting to re-run a test like that on a larger box to see if it was a fluke or not.
Comment Actions
@Ray Molenkamp (LazyDodo): those are very nice speedups, would be great to see this confirmed. Getting a 6% speedup in a scene like that is usually hard work.
Agreed about the overflow.
| intern/cycles/util/util_task.cpp | ||
|---|---|---|
| 207–226 | Would prefer this code to be structured a bit simpler: size_t i = 0;
int num_groups = system_cpu_group_count();
for (int group = 0; group < num_groups && i < threads.size(); group++) {
int system_cpu_group_thread_count(group);
for (int thread = 0; thread < threads_per_group && i < threads.size(); thread++) {
threads[i] = new thread(...);
}
} | |
Comment Actions
Wow, that's quite measurable speedup!
However, a bit skeptical to just enable such affinity without extra tests. Will do it on various hardware in the studio and if nothing bad happens will enable that.
| intern/cycles/util/util_task.cpp | ||
|---|---|---|
| 207–226 | That was mainly a safety feature to prevent possible regression on nn-NUMA machine, to which i did not have access to when was working on orignial patch.. But now i tested it on laptop and it all works. Will submit simplified patch soon. | |
| intern/cycles/util/util_thread.cpp | ||
| 49 | That's a good catch! | |
| intern/cycles/util/util_task.cpp | ||
|---|---|---|
| 207–226 | I just realized that my code doesn't handle the more threads than processors case, but still might be simpler with that I guess. | |
| intern/cycles/util/util_task.cpp | ||
|---|---|---|
| 207–226 | Not sure what you mean. Thread is a logical processor an included into system_cpu_group_thread_count(). So should be fine. | |
| intern/cycles/util/util_task.cpp | ||
|---|---|---|
| 207–226 | Sorry I wasn't clear, I meant the user specified num_threads bigger than the number of system threads. Not that this is very useful. | |
| intern/cycles/util/util_task.cpp | ||
|---|---|---|
| 207–226 | Ok, got it now. Yes, that's something i'm addressing atm. Think it's fine to do some simple-to-read but not very efficient way of dealing with this and just use residual number of threads for the last group (instead of the group size). | |
Comment Actions
Address review feedback.
Constant thread affinity i'll test later this weekend,
would be better to commit it separately anyway.
Comment Actions
Forgot to mentionm, inbalanced groups are now solved as well. So currently my TODO list about this patch is empty.
Comment Actions
Seems fine except the Windows 7 minimum requirement, we still support Vista so I guess this needs a solution before it can be committed.
Comment Actions
Eeeeh, sorry of the spam...
One thing which is separate form code and everything: would need to clarify whether this breaks Vista, whether we support Vista or whether we need to dlopen something to prevent compatibility issues.
Comment Actions
I don't have a vista box to test it on, but i'm about 99.9999% sure if one would run this code on vista ,at process creation time you'd get a popup telling you
"Procedure entry point procedure entry point getactiveprocessorgroupcount could not be located in the dynamic link library kernel32.dll" (or any of the other win7+ apis) witht the process terminating after you click ok.
While there might not be many vista users, Vista shares the kernel with windows server 2008, which is probably worth supporting (as in groups don't work, but atleast blender should start) so yeah lower _WIN32_WINNT back to 0x600 and get the functions you need using LoadLibrary/GetProcAddress.
Comment Actions
Use GetProcAddress() to get Group/NUMA specified functions
(well, it's only Group for now, NUMA is on TODO)
Comment Actions
Well it is not really on topic here blender.org stats indicate < 0.7% marketshare for vista.
I personally find testing vista timeconsuming and would really like to see it dropped as a platform.
http://windows.microsoft.com/en-gb/windows/lifecycle would make us support it till next summer not sure I am up for that
| intern/cycles/util/util_windows.cpp | ||
|---|---|---|
| 71 ↗ | (On Diff #6857) | typo SUMBOL => SYMBOL. |
| intern/cycles/util/util_windows.cpp | ||
|---|---|---|
| 24 ↗ | (On Diff #6857) | This should be wrapped with #if _WIN32_WINNT < 0x0601 as well, otherwise will get compile error once we've switched to Win7+. |