-
Notifications
You must be signed in to change notification settings - Fork 201
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
Proof of Concept: Release GVL in Statement#step #528
base: main
Are you sure you want to change the base?
Conversation
This allows other threads to execute Ruby code (including performing more work sqlite operations if on another DB connection). This should significantly improve multithreaded performance, however will add overhead to single-threaded performance. This required wrapping all callbacks which can happen within statement execution with rb_thread_call_with_gvl.
Do you think it might be useful to allow users to make this decision, e.g. via a Allowing folks who are knowledgeable about their production runtime to decide whether to release the GVL or not might still be a huge step forward, and allow higher-level abstractions (like the Rails database adapter) to evolve their own policies. |
A configuration option is probably the safest bet, at least for I'm not sure immediately why this is failing CI on the Windows |
Something we mentioned in the past was to only release the GVL around the first call to |
In the general case this isn't true, unfortunately. Building on the benchmark in my gist: db.prepare("SELECT * FROM t2 WHERE b>=2 AND b<500") do |stmt|
10.times do
puts Benchmark.realtime { stmt.step } * 1000
end
end
stmt.step just allows sqlite's VM to continue running, so it depends whether it has work to do between the first row returned and the second (ie. continue scanning the table). It's possible that releasing once is a good heuristic though for queries which have an index? |
In the Extralite gem, @noteflakes iterated on this problem and landed with a configurable
— https://github.com/digital-fabric/extralite?tab=readme-ov-file#the-ruby-gvl This is the pull request that added the feature, and this is the pull request that was closed in favor of this approach. Just offering some additional context and food for thought. |
Refs #287, but I don't think this is ready to merge. As described below the performance impact is very significant. I think this is something the library should do, but it's a penalty that's hard to just eat.
This allows other threads to execute Ruby code (including performing more work sqlite operations if on another DB connection). This should significantly improve multithreaded performance, however as @tenderlove correctly predicted in #287 adds significant overhead to single-threaded performance.
This required wrapping all callbacks which can happen within statement execution with rb_thread_call_with_gvl.
I've written two benchmarks, one which shows why this could be a good idea and one which shows why it's a bad idea. It's unfortunately not clear cut.
The good
https://gist.github.com/jhawthorn/ae84ee2723ff4930344778cfe40205a2
I made a benchmark based on making sqlite actually do work (a poorly optimized query) before returning, and then having multiple threads perform the same operation. This shows a very significant speed up as we are actually able to run the sqlite operations in parallel.
main
This branch:
The bad
https://gist.github.com/jhawthorn/492b6a9c5fb6bebe1ebfe985f52495de
When running a completely trivial query in a single thread, this is about 60% slower due to the need to release and re-acquire the GVL on every call to
Statement#step
. I don't think there's an easy way around this as even if our API was a more "bulk" operation (ie "return all rows" rather than "return one row") I believe we'll need the GVL every time we want to allocate a Ruby string and our memory buffer returned from SQLite is only good until our next call tosqlite3_step
.I do think this is capturing the absolute worst case as the query is completely trivial and doesn't even make allocations other than the one array per-row.
So that's where we're at. There might be some sort of compromise we could do like making the GVL releasing optional or based on a heuristic. I haven't yet investigated any way to make this faster, but it does seem unlikely we'll find a way to do this without a performance penalty.
TODO
rb_thread_call_without_gvl