Skip to content
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

Closed
wants to merge 3 commits into from
Closed

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Aug 5, 2020

No description provided.

@beauby beauby force-pushed the cmake-build branch 10 times, most recently from 374feaf to c9ff4c7 Compare August 5, 2020 14:52
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@beauby beauby force-pushed the cmake-build branch 3 times, most recently from 162e33a to 4668213 Compare August 10, 2020 16:20
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@beauby beauby force-pushed the cmake-build branch 3 times, most recently from 59bcece to 3869345 Compare August 14, 2020 19:33
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@facebook-github-bot
Copy link
Contributor

@beauby merged this pull request in a8e4c5e.

facebook-github-bot pushed a commit that referenced this pull request Aug 15, 2020
Summary:
Depends on #1313.

Pull Request resolved: #1315

Reviewed By: mdouze

Differential Revision: D23148557

Pulled By: beauby

fbshipit-source-id: 0a35f17d22aa04db6bd1c16cfc5ff8eee28f1f74
sadroeck pushed a commit to singlestore/faiss that referenced this pull request Jun 11, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants