Only release the GVL for row fetches that can actually block - #1457
Open
nuclearspike wants to merge 1 commit into
Open
Only release the GVL for row fetches that can actually block#1457nuclearspike wants to merge 1 commit into
nuclearspike wants to merge 1 commit into
Conversation
`rb_mysql_result_fetch_row` and `rb_mysql_result_fetch_row_stmt` release and
re-acquire the GVL around every single row, via `rb_thread_call_without_gvl`.
Profiling a 20,000-row query with macOS `sample` put 2,144 of 10,176 stack
samples inside that path -- and 2 in the actual `mysql_fetch_row`. The rest was
`setjmp` / `sigprocmask` / `__sigaltstack`: the cost of entering and leaving a
blocking region 20,000 times for a call that, on a buffered result, walks memory
the client library has already filled.
The comment above `nogvl_fetch_row` says there is "no reliable way for us to
tell" whether a fetch will hit the network. `wrapper->is_streaming` is that way:
* client.c: `mysql_use_result` (unbuffered) is called in exactly one place,
guarded by `stream == true` from `current_query_options`; every other path
uses `mysql_store_result`. The same options hash is then duplicated into the
Result, where it sets `wrapper->is_streaming`.
* statement.c: `is_streaming` is read from the merged options hash, and
`mysql_stmt_store_result` runs whenever it is false. That same hash is
passed to `rb_mysql_result_to_obj`.
So `is_streaming == false` implies the rows are already in client memory. The
one asymmetry -- `Client#store_result` called while the options say
`stream: true` -- yields a buffered result that still reports as streaming, and
therefore still releases the GVL. That is the harmless direction.
Streaming results release the GVL exactly as before. Buffered results call
`mysql_fetch_row` / `nogvl_stmt_fetch` directly, and the buffered iteration loop
calls `rb_thread_schedule()` every 8192 rows instead.
The periodic yield is placed on the branch that actually fetches a row, so
re-iterating an already-cached result keeps its current scheduling behaviour,
and the streaming loop is untouched.
Holding the GVL across a fetch means other Ruby threads wait longer. With a
block, iteration returns to the VM each row and Ruby's ordinary preemption still
applies. Without a block the loop is pure C: nothing returns to the VM, so
without some yield the GVL would be held for the entire materialisation, and the
longest another thread could be kept waiting would grow with the result size
rather than being bounded. Measured that way -- a second thread looping on
`sleep 0.001`, longest interval it was kept off the GVL, query issued outside
the measured window, median of 3, cache_rows: false -- an earlier revision of this patch without
the periodic yield reached 269 ms at 1.6M rows and 286 ms at 3.2M and was still
climbing. Under a threaded server that is a real latency regression.
Hence the yield. With it, the same measurement no longer grows with result
size, staying a small bounded delay of the same order as master's:
| rows | | master | patch |
|---|---|---|---|
| 400,000 | `each { }` | 1.3 ms | 2.4 ms |
| 400,000 | `each` | 1.3 ms | 2.2 ms |
| 3,200,000 | `each { }` | 1.3 ms | 2.5 ms |
| 3,200,000 | `each` | 1.3 ms | 2.5 ms |
The bound does not grow with result size -- 2.5 ms is the worst case at 3.2M
rows, against master's 1.3 ms -- and the same run shows the materialisation
itself at 221 ms vs master's 741 ms on that shape (`cache_rows: false`,
median of 3).
On the interval: 8192 rows is 0.6-1.0 ms of materialisation on the shapes above,
taken from the same runs' total iteration time divided by row count. It is
coarse enough that the per-row handoff this commit removes does not come back,
and it does not scale with result size, so for similar row shapes the bound
should not grow with row count.
No new specs: no row value changes, and the scheduling property above is
measured rather than asserted -- a timing-sensitive spec for it would be flaky
in CI.
`rb_thread_schedule()` is the only API newly used here. It is declared in
`ruby/internal/intern/thread.h` among the installed public headers, so I did not
add a `have_func` guard for it. On the range: I checked 3.3 and 2.6, and it is
declared in both. I did not check 2.0-2.5, so I cannot tell you from my own
testing whether it spans the gem's declared `required_ruby_version` of
>= 2.0.0. If that matters, it is a one-line `have_func` guard and I am happy
to add it.
20,000 rows from a real application table (10 columns: bigints, varchars, a
DATETIME), `as: :array` with `database_timezone: :utc` -- the configuration
Rails' mysql2 adapter uses. Each sample is the min of 9 runs in one process;
9 samples per arm, arms interleaved to cancel drift.
| | median | range |
|---|---|---|
| master | 30.44 ms | 29.31 - 34.84 |
| patch | 25.21 ms | 23.89 - 28.63 |
-17.2%, with no overlap between the two distributions.
Ruby 3.3.10, MySQL 9.6.0 (Homebrew, libmysqlclient.24), macOS 26.5 arm64,
Apple clang 21. The rspec suite was run there and is green apart from three failures
already present on the base commit: `client_spec.rb:101`, `:729`, and `:740`
-- the reconnect group.
An earlier revision of this change -- before the periodic yield existed -- was
also built and exercised inside a Rails application on a Heroku Linux dyno
(amd64) against MySQL 9.6. The revision in this PR, including the yield, has
been built and tested only on macOS.
Not tested: MariaDB, Windows, 32-bit platforms, or other Ruby versions. The
buffering argument above rests on libmysqlclient's behaviour for
`mysql_store_result` / `mysql_stmt_store_result`; MariaDB's client library is
the untested case that matters most for it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
rb_mysql_result_fetch_rowandrb_mysql_result_fetch_row_stmtrelease andre-acquire the GVL around every single row, via
rb_thread_call_without_gvl.Profiling a 20,000-row query with macOS
sampleput 2,144 of 10,176 stacksamples inside that path -- and 2 in the actual
mysql_fetch_row. The rest wassetjmp/sigprocmask/__sigaltstack: the cost of entering and leaving ablocking region 20,000 times for a call that, on a buffered result, walks memory
the client library has already filled.
The comment above
nogvl_fetch_rowsays there is "no reliable way for us totell" whether a fetch will hit the network.
wrapper->is_streamingis that way:mysql_use_result(unbuffered) is called in exactly one place,guarded by
stream == truefromcurrent_query_options; every other pathuses
mysql_store_result. The same options hash is then duplicated into theResult, where it sets
wrapper->is_streaming.is_streamingis read from the merged options hash, andmysql_stmt_store_resultruns whenever it is false. That same hash ispassed to
rb_mysql_result_to_obj.So
is_streaming == falseimplies the rows are already in client memory. Theone asymmetry --
Client#store_resultcalled while the options saystream: true-- yields a buffered result that still reports as streaming, andtherefore still releases the GVL. That is the harmless direction.
Streaming results release the GVL exactly as before. Buffered results call
mysql_fetch_row/nogvl_stmt_fetchdirectly, and the buffered iteration loopcalls
rb_thread_schedule()every 8192 rows instead.The periodic yield is placed on the branch that actually fetches a row, so
re-iterating an already-cached result keeps its current scheduling behaviour,
and the streaming loop is untouched.
Holding the GVL across a fetch means other Ruby threads wait longer. With a
block, iteration returns to the VM each row and Ruby's ordinary preemption still
applies. Without a block the loop is pure C: nothing returns to the VM, so
without some yield the GVL would be held for the entire materialisation, and the
longest another thread could be kept waiting would grow with the result size
rather than being bounded. Measured that way -- a second thread looping on
sleep 0.001, longest interval it was kept off the GVL, query issued outsidethe measured window, median of 3, cache_rows: false -- an earlier revision of this patch without
the periodic yield reached 269 ms at 1.6M rows and 286 ms at 3.2M and was still
climbing. Under a threaded server that is a real latency regression.
Hence the yield. With it, the same measurement no longer grows with result
size, staying a small bounded delay of the same order as master's:
each { }eacheach { }eachThe bound does not grow with result size -- 2.5 ms is the worst case at 3.2M
rows, against master's 1.3 ms -- and the same run shows the materialisation
itself at 221 ms vs master's 741 ms on that shape (
cache_rows: false,median of 3).
On the interval: 8192 rows is 0.6-1.0 ms of materialisation on the shapes above,
taken from the same runs' total iteration time divided by row count. It is
coarse enough that the per-row handoff this commit removes does not come back,
and it does not scale with result size, so for similar row shapes the bound
should not grow with row count.
No new specs: no row value changes, and the scheduling property above is
measured rather than asserted -- a timing-sensitive spec for it would be flaky
in CI.
rb_thread_schedule()is the only API newly used here. It is declared inruby/internal/intern/thread.hamong the installed public headers, so I did notadd a
have_funcguard for it. On the range: I checked 3.3 and 2.6, and it isdeclared in both. I did not check 2.0-2.5, so I cannot tell you from my own
testing whether it spans the gem's declared
required_ruby_versionof20,000 rows from a real application table (10 columns: bigints, varchars, a
DATETIME),
as: :arraywithdatabase_timezone: :utc-- the configurationRails' mysql2 adapter uses. Each sample is the min of 9 runs in one process;
9 samples per arm, arms interleaved to cancel drift.
-17.2%, with no overlap between the two distributions.
Ruby 3.3.10, MySQL 9.6.0 (Homebrew, libmysqlclient.24), macOS 26.5 arm64,
Apple clang 21. The rspec suite was run there and is green apart from three failures
already present on the base commit:
client_spec.rb:101,:729, and:740-- the reconnect group.
An earlier revision of this change -- before the periodic yield existed -- was
also built and exercised inside a Rails application on a Heroku Linux dyno
(amd64) against MySQL 9.6. The revision in this PR, including the yield, has
been built and tested only on macOS.
Not tested: MariaDB, Windows, 32-bit platforms, or other Ruby versions. The
buffering argument above rests on libmysqlclient's behaviour for
mysql_store_result/mysql_stmt_store_result; MariaDB's client library isthe untested case that matters most for it.
Related but not addressed here: #1033 (
next_resultholds the GVL during a network call) is a different call site with a real network read; this PR only changes fetches from already-buffered results.🤖 Generated with Claude Code