-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Move build to CMake #1313
Move build to CMake #1313
Conversation
374feaf
to
c9ff4c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beauby has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beauby has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beauby has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
162e33a
to
4668213
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beauby has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
59bcece
to
3869345
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beauby has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beauby has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: FAISS uses OpenMP to spawn additional threads to parallelize many of its operations. However, this model doesn't work well with already multi-threaded programs like memsqld because the additional threads can cause issues without caution. In https://github.com/facebookresearch/faiss/wiki/Threads-and-asynchronous-calls#internal-threading, FAISS suggests two ways to disable its internal threads 1. Adjusting environment variable `OMP_NUM_THREADS`. Is is tron to have dependency on environment vairables and this can introduce many deployment issues; 2. Or at any time by calling `omp_set_num_threads(). This can be too late in certain cases. During debugging, I noticed that some threads are spawned during the library startup when the loader loads the library, and this is way before `main()` gets called. FAISS used to have an option `--disable-openmp` to disable OpenMP. However, it was removed in facebookresearch/faiss#1313 when they migrate to CMake. This diff disables OpenMP by hijacking a dummy implementation of `<omp.h>` to FAISS. Also, this diff changes the compiler from GCC to Clang to be consistent with memsqld, though I don't think it makes much difference here. Test Plan: .psyduck Reviewers: eli, szupo, zhou Subscribers: engineering-list Differential Revision: https://grizzly.internal.memcompute.com/D63501
No description provided.