search

rivetTDA / rivet

RIVET is a tool for Topological Data Analysis, in particular two-parameter persistent homology.
GNU General Public License v3.0
73 stars 24 forks source link

Added OpenMP multi-threading to Presentation::minimize() #125

Closed davidt0x closed 6 years ago

davidt0x commented 6 years ago

A few simple changes to get basic multi-threading support Presentation::minimize(). I kept it using simple OpenMP static scheduling because dynamic severely hurt performance. Not what I expected so more profiling would need to be done to see why. Also added a command line option to override the default number of the threads.

mlesnick commented 6 years ago

I tested this on my MacBook and OpenMP is not detected. I expect that this means either that CMakeLists.txt or the RVIET installation instructions need to be updated, but I'd like to hear from the experts. @xoltar, any thoughts?

davidt0x commented 6 years ago

What version of cmake? This might be related: https://stackoverflow.com/questions/49738961/cmake-support-for-openmp-on-macos-high-sierra

mlesnick commented 6 years ago

I am running cmake 3.11.4. If this is an issue with cmake that will be fixed in a coming release, then I suppose we could just merge, create an issue, and wait for the fix. In the meantime, this works fine on OS X; we just get error messages when running cmake, and the build defaults to just use a single core. Dave and @xoltar, please let me know what you suggest.

BTW, I have tested on Ubuntu as well and had no problems so far. On my machine (8 cores), the augmented arrangement computation for the 600 points noisy circle example (H1, 50x50 bins) went from ~9.5 minutes to ~6.25 minutes. This is a very decent improvement, considering that minimizing the presentation is only one of two expensive steps here.

mlesnick commented 6 years ago

Apparently cmake 3.12.0 is available as a release candidate. I downloaded OS X binaries and tried running this. It didn't help. If I understand correctly, this version incorporates the patch that is mentioned in the link above, so I'm not sure what the problem is.

mlesnick commented 6 years ago

Here are the specific errors I get when I run cmake (same for either 3.11.4 or 3.12.0) -- Could NOT find OpenMP_C (missing: OpenMP_C_FLAGS OpenMP_C_LIB_NAMES) -- Could NOT find OpenMP_CXX (missing: OpenMP_CXX_FLAGS OpenMP_CXX_LIB_NAMES) -- Could NOT find OpenMP (missing: OpenMP_C_FOUND OpenMP_CXX_FOUND)

davidt0x commented 6 years ago

What version of clang is it?

mlesnick commented 6 years ago

It's Apple LLVM version 8.1.0 (clang-802.0.42).

On Mon, Jul 16, 2018 at 11:02 AM David Turner notifications@github.com wrote:

What version of clang is it?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rivetTDA/rivet/pull/125#issuecomment-405277863, or mute the thread https://github.com/notifications/unsubscribe-auth/AL-lBNYhpIuBZrtdDQgR3kjjtXOadFTjks5uHKsSgaJpZM4VIbUc .

davidt0x commented 6 years ago

See this post:

https://stackoverflow.com/questions/44380459/is-openmp-available-in-high-sierra-llvm

It seems Apple's fork of LLVM is missing OpenMP out of the box. Seems odd to me. You can try the workarounds. I would do it myself but I don't have a mac to try it on.

mlesnick commented 6 years ago

Some relevant info can be found here. After looking at this and the links Dave posted above, I've gathered that to get this to work on OS X, the first step is to install the OpenMP runtime library via homebrew, using the command:

brew install libomp

On top of that, CMakeLists.txt needs to be modified and, perhaps, one wants to run CMake 3.12.0, which includes a patch to help CMake find what it's supposed to. The link above gives some hints about what that modification should be, but we haven't managed to make it work yet. It seems that the patch and the instructions given at the links assume that CMake is begin used with the Target system.

xoltar commented 6 years ago

Mac Clang has never supported OpenMP as far as I know. Maybe the situation has changed in high sierra, I haven't looked. People have complained about it a lot but it's been that way for a long time. You can install brew's version of llvm that has a clang with openmp, or else use g++.

mlesnick commented 6 years ago

Since this change works well on a Linux, and fails in an innocuous way on OS X (just an error message when you run cmake, and then a normal non-parallel build), I would like to merge the change and create an issue describing the trouble with OS X and OpenMP. If there are no objections, I will do this in the next few days. It should also be easy to get rid of the error message when running cmake.