search

ilpincy / argos3

A parallel, multi-engine simulator for heterogeneous swarm robotics
http://www.argos-sim.info/
268 stars 121 forks source link

Add ability for ARGoS to pin threads to cores #175

Closed jharwell closed 2 years ago

jharwell commented 3 years ago

Also moved common thread functionality to a CSpaceMultiThread base class to reduce duplication between CSpaceMultiThreadBalanceLength and CSpaceMultiThreadBalanceQuantity, which contains:

ilpincy commented 3 years ago

This great on Linux! Unfortunately it does not seem to compile on Mac OSX. I am trying to spot the issue, hoping I don't have to resort to conditional compilation. Nice work on the refactoring the code too!

ilpincy commented 3 years ago

Alright. This seems to be the thing: http://www.hybridkernel.com/2015/01/18/binding_threads_to_cores_osx.html.

I would say that, for the time being, we keep thread affinity as a Linux-only feature. Would you mind adding conditional compilation statements around the relevant portions of the code?

jharwell commented 3 years ago

Added guards around the actual pinning call, and a warning if you try to pin threads on OSX, which is in keeping with the principle of least surprise.

I think that is the only location the guards are needed, but I don't have a Mac, so I'm not 100% sure.

jharwell commented 3 years ago

@ilpincy Can this be merged ?

jharwell commented 3 years ago

@ilpincy @allsey87 If there are no objections, I will merge this on Monday 10/18.

ilpincy commented 3 years ago

How much is this tested? I didn't have time to test it on Mac yet – does it work on Ubuntu / OpenSUSE / Arch / etc?

jharwell commented 3 years ago

Per the conversation above, we decided to keep it a linux-only feature; enabling this does nothing on OSX, so no testing is needed. The crucial bit is the pthread_setaffinity_np call which is POSIX, so if it works on ubuntu (which it does), then it should work on every POSIX system, including Arch, OpenSUSE, etc.

allsey87 commented 3 years ago

I (and perhaps also @ilpincy) find these sorts of changes a bit intimidating since although we now have a testing infrastructure set up, there are still very few tests in place. Writing these tests is not complicated but it does take a bit of time. Perhaps if you could help out with the testing-writing effort, we would all be a bit more comfortable with merging changes like this...

ilpincy commented 3 years ago

I am sure this is a good change. It is just a question for me to a) test that it works on Mac as intended (i.e., that compilation is correctly configured to bypass these calls) and b) That it works on a sufficiently wide set of distributions. For example, older versions of GCC might not support these extensions, and on (say) Ubuntu 16 one might need to add a feature test to prevent compilation issues. If all of these tests pass, we can merge.

jharwell commented 3 years ago

@allsey87 What kind of tests would be helpful for increasing confidence about merging this and possible future changes of this nature?

ilpincy commented 3 years ago
jharwell commented 3 years ago

16.04 comes with glibc 2.27, and the man page for pthread_set_affinity_np() says that all glibc versions since 2.3.4 (really 2.03.4) contain this functionality, so I don't think it should be an issue on 16.04.

Where would I document this? In the README?

ilpincy commented 3 years ago

I would just try to compile it on Ubuntu 16.04, and if nothing noteworthy comes up, there's no need to document anything. Are you able to do it? If not, I'll see if I can set up an Ubuntu VM this week.

jharwell commented 3 years ago

Yes I can do that.

ilpincy commented 3 years ago

Fantastic, thanks!

allsey87 commented 3 years ago

@ilpincy didn't we decide that we were only supporting the last two LTS releases of Ubuntu? Even Ubuntu themselves no longer support 16.04 as of 6 months ago: https://wiki.ubuntu.com/Releases

@jharwell we need tests that check whether sensor readings match their expected values and that actuators effect the simulation as they are expected to. By having a broad set of tests, we will be able to automatically identify any problematic PRs.

jharwell commented 2 years ago

@ilpincy Is this OK to merge?