Thread pool warning

[malric]

(please ignore if we are not supposed to report bugs on develop)

With 97.0.1-55-g73edcbc there is a message

Code: Select all

Warning: Set ThreadPool workers to 4, but there are just 1 cores!

(whole infolog here: http://pastebin.com/yVcDMk8W). A bit previously it is reported I have 1 physical core and 8 logical cores, and as far as I know the processors has 4 cores (and 2 threads per core), so I would say there should be at least 4 workers set.

By checking the current code at InitThreadPool it seems that after the warning the worker count is set to numCores (which is always 1 as it seems defined in the header), so it does not seem to be a good value.

[jK]

meh .... I used a code from stackoverflow as base for my physical core count detection code. Seems it's broken for Intels.

edit: removed any vendor specific code, I hope this works.
PS: the idea of the change is that it's bad to spawn 14+1 threads on a hexcore + HTT. I assume it's the main cause why many ppl request WorkerThreadsSpinTime=0.

[malric]

Yes, the code does not seem to do what it should, and still it does not work for me (reports now 16 threads when, including HT I have only 8 threads).

According to http://www.intel.com/content/dam/www/pu ... manual.pdf, it seems that the field that you use is (check page 220 of the pdf):

Bits 23-16: Maximum number of addressable IDs for logical processors in this physical package

In the thread at https://software.intel.com/en-us/forums/topic/287414 some code is mentioned that should solve the problem (which I think is also used in 0ad). Same/similar code is also found at https://github.com/0ad/0ad/blob/457c405 ... pology.cpp

[jK]

Are you willing to supply a ~100loc patch?
I don't got a testing platform for it, also after a fast glance at the intel papers & 0ad code (seems you need at least basic ACPI codes) I lost interest in coding it (esp. coding it blind).
If none is willing to supply a patch, my workaround would be a simple HTT support check and then physicalCores:=logicalCores/2.

[malric]

Yes, sure, will do that sometime this week. I can test it on 2 systems, so hope it will help check it behaves as it should.

[Anarchid]

I've got HT, willing to test.

[Silentwings]

Same.

[Google_Frog]

Is it pointless to test performance until this is fixed? Should I expect a worst case performance improvement by disabling multithreading?

[malric]

If you want to continue testing you could use a build before this change was introduced (I think 97.0.1-24-g93bd67d/).

But keep in mind that in that case (previous builds), the default number of threads will be created based on the logical cores (logical cores are including the cores that exist due to hyperthreading). It might be that in case of spring (I did not check/test/benchmark that) the hyperthreading does not work well, so you get slowdowns.

Not sure if someone tried (jk, etc), it would be very interesting if you could run 2 tests, one with the default value for older builds, and one where people set WorkerThreadCount to the numbers of "real" cores they have. In this way we could assess the impact of having "too many cores".

PS: I created a bug for this at: http://springrts.com/mantis/view.php?id=4445, forums are good to identify problems, but I think a proper bug might collect information/discussions better.