-
Notifications
You must be signed in to change notification settings - Fork 8
Add support for .freeze
for a read-only mode that releases the GVL
#7
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
base: master
Are you sure you want to change the base?
Conversation
cfde05b
to
c9dca0e
Compare
Hi @atesgoral, thanks for the PR! However, this doesn't seem safe to do without more locking (as https://github.com/facebookresearch/faiss/wiki/Threads-and-asynchronous-calls |
@ankane are you sure that's true? According to the link you sent:
I'm not familiar with the Python extension, but it seems like we should be in the same boat? Am I missing something? |
Will check out the Python code, but this causes the test to consume all available CPU and hang (which doesn't happen when the GVL is enabled). --- a/test/index_test.rb
+++ b/test/index_test.rb
@@ -279,17 +279,18 @@ class IndexTest < Minitest::Test
[1, 1, 2, 1],
[5, 4, 6, 5],
[1, 2, 1, 2]
- ]
+ ] * 100
index = Faiss::IndexFlatL2.new(4)
index.add(objects)
concurrency = 0
max_concurrency = 0
- threads = 2.times.map {
+ threads = 100.times.map {
Thread.new {
concurrency += 1
max_concurrency = [max_concurrency, concurrency].max
+ index.add(objects)
index.search(objects, 3)
concurrency -= 1
} |
@ankane Thanks for having a look. I didn't need more data or 100 iters to get it to lock. I haven't looked at faiss in detail yet but I see the word "lock" all over the repo. This is enough to cause a deadlock(?): diff --git a/test/index_test.rb b/test/index_test.rb
index 64e093b..095b8ea 100644
--- a/test/index_test.rb
+++ b/test/index_test.rb
@@ -290,7 +290,13 @@ class IndexTest < Minitest::Test
Thread.new {
concurrency += 1
max_concurrency = [max_concurrency, concurrency].max
+ puts "adding"
+ index.add(objects)
+ puts "added"
+ sleep(10)
+ puts "searching"
index.search(objects, 3)
+ puts "searched"
concurrency -= 1
}
} I say "deadlock(?)" because it doesn't act like an forever deadlock, but something gives eventually and the tests ends with a success. 🤔 |
keeping 👀 on this |
@ankane I can rejig this to add a read-only mode to an index so it can be used with GVL release in read-only use cases. Option 1
Option 2
Option 3
|
Co-authored-by: Ufuk Kayserilioglu <[email protected]> Co-authored-by: Aaron Patterson <[email protected]> Move without_gvl to utils Add parallelism test Simplify config
a016bd4
to
525069c
Compare
I updated this PR to hang the decision to drop the GVL off of |
525069c
to
0634cc5
Compare
.freeze
for a read-only mode that releases the GVL
0634cc5
to
3e3b325
Compare
|
||
self.search(n, objects.read_ptr(), k, distances.write_ptr(), labels.write_ptr()); | ||
if (wrapper.is_frozen()) { | ||
without_gvl([&] { |
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.
This would be helfpul to add to Rice. Pybind does it like this:
https://pybind11.readthedocs.io/en/stable/advanced/misc.html#global-interpreter-lock-gil
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.
I'd be happy to add it to Rice! I don't think it's easy to write a similar API to the pybind11 one since the Ruby ones all take a callback, but I can add something like this without_gvl
easily.
Any suggestions for where to put it? Not familiar with the code structure of Rice
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.
Created a PR for Rice here: ruby-rice/rice#313
Hi @tavianator, thanks for sharing. I like the simplicity of this approach. Can you share a benchmark of the performance improvement? Also, I agree with @cfis that it'd be nice to add the GVL code to Rice. |
Add a new check to all mutating methods that we're not operating on a frozen Index. After that, it should be safe to drop the GVL for search on a frozen Index, since nothing can be mutating it in parallel.
@ankane It looks like a win. tl:dr;
I got Claude Code vibe-code these benchmark scripts: 2367f1c Output of the more intense one:
|
@cfis Is a new Rice release with the GVL-free function calls on the horizon? |
Yes, will try to push out a release this week. Have been updating documentation first, but almost done with that. |
Release GVL for parallel search operations
This PR improves multi-threading performance by releasing Ruby's Global VM Lock (GVL) during Faiss search operations, allowing multiple threads to perform searches in parallel.
Changes
rb_thread_call_without_gvl
to allow parallel executionUsage
Notes