Skip to content

Make the matmul, miniexpr and Menudet prefilters unable to need the GIL - #774

Open
FabianIsensee wants to merge 1 commit into
Blosc:mainfrom
FabianIsensee:fix-matmul-prefilter-nogil
Open

FabianIsensee wants to merge 1 commit into
Blosc:mainfrom
FabianIsensee:fix-matmul-prefilter-nogil

Conversation

@FabianIsensee

Copy link
Copy Markdown
Contributor

C-Blosc2 calls prefilters from its worker threads when nthreads > 1, and from the calling thread without the GIL when a compression releases it (set_releasegil(True), _can_compress_without_gil()). _check_cparams() allows that for the miniexpr, matmul and Menudet prefilters because they are C-only, but two of them were not: the error paths of miniexpr_prefilter/aux_miniexpr() and matmul_prefilter/aux_matmul() raised Python exceptions. Cython wraps a raise in a nogil function in an implicit with gil, and the (not nogil) wrappers then called PyErr_Occurred() without the GIL. In addition, all of these prefilters decompress their operands themselves, so a codec or filter registered from Python ran on threads without the GIL.

What still happens on main (5ff9823, after #741), with an operand chunk whose header parses but whose blocks cannot be read (block offsets past the chunk):

  • blosc2.matmul(x1, x2, ...) through the fast path: deadlock with 4 threads (the caller holds the GIL inside the compression and the worker that wants it for the raise waits forever), segfault under set_releasegil(True) with 1 and 4 threads.
  • Lazy expressions and reductions with BLOSC2_EXPRESSION_EVALUATOR=legacy (the miniexpr prefilter): the same deadlocks and segfaults (sum() included, whose prefilter compression now also releases the GIL), and SystemError: <class 'ValueError'> returned a result with an exception set for sum() with one thread.
  • With the default typed evaluator, the Menudet prefilter that Introduce Menudet 0.1 draft for portable array and table computation #741 added reports the same broken chunk cleanly (MenudetError), so those cases are not part of this PR any more.

Operands compressed with a codec or filter registered from Python, all segfaults on main: matmul fast path, legacy expressions and reductions, and the typed evaluator's Menudet prefilter (1-D operands; the reduction even with one thread, since its driver runs without the GIL), all with 4 threads; and (x1 + x2)[:] on >= 2M items through the parallel native graph map, whose workers also decode the inputs themselves. With one thread the matmul and legacy paths fail with an error, because the callbacks need an SChunk that the prefilter's decompression context does not have.

dtypes: the raise ValueError("Unsupported dtype") in matmul_prefilter is not reachable through blosc2.matmul(), whose fast path wants dtype.kind == "f". But float16 and longdouble are kind "f" too, and aux_matmul() computed them with the float64 kernel: NaN/garbage results, and for float16 reads of 4x the operand block buffers and writes of 4x the output block. Calling _set_pref_matmul() directly also accepted int8/int16 (int64 kernel, same overrun), complex, bool, unsigned (the raise) and mixed or structured dtypes.

What this PR does:

  • miniexpr_prefilter, matmul_prefilter, aux_miniexpr(), aux_matmul() and matmul_block_kernel() are noexcept nogil and contain no raise and no Python object. These two and the Menudet prefilter use cdivision, because Cython's zero-division checks take the GIL too (all divisors there are shapes and strides > 0). In the generated C, none of the three prefilters and their helpers calls the Python C API any more.
  • aux_miniexpr() keeps its first error in its worker slot (me_worker_s.error, like the Menudet workers' status) and returns -1; _store_data() (writes) and SChunk._prefilter_data() (reductions, where C-Blosc2 ignores the return code because the output is disposable) raise it as RuntimeError("miniexpr: ..."). aux_matmul() frees what it allocated (its 2-pointer tables are on the stack now) and returns -1; blosc2.matmul() then falls back to its chunked path as for other fast-path failures.
  • _set_pref_matmul() raises TypeError before setting the prefilter unless both operands and the output have one dtype out of float32, float64, int32, int64 (native byte order); matmul_prefilter returns -1 for anything else. _matmul_can_use_fast_path() takes float32/float64 only.
  • Operands whose codec or filter is registered from Python (judged by the array's cparams, as _check_cparams() does) are declined by the routing gates: the matmul fast-path predicate, _set_pref_expr(), _set_pref_matmul(), menudet_prefilter_inputs_eligible() and native_graph._compressed_execution_layout(). Those operations take their other paths, which decompress with the GIL.

Not in this PR: a chunk written with other cparams than the array's (e.g. through update_chunk()) is not caught by the gate. A guard inside the Python codec/filter trampolines would catch every GIL-free call, but PyGILState_Check() is not in the Limited API that the abi3 wheels use.

Tests:

  • tests/ndarray/test_prefilter_errors.py: each case runs in a child interpreter (python test_prefilter_errors.py <case> <args>, as tests/test_python_codec_threads.py does), whose faulthandler ends it after 25 s with the stacks of its threads (plus a 30 s subprocess timeout), so a regression fails instead of hanging CI or killing the worker. On WASM (no subprocesses, no threads) the single-thread cases with the GIL run in-process and the others are skipped.
    • test_prefilter_error_raises (matmul, legacy compute(), legacy sum(); 1 and 4 threads; with and without set_releasegil(True)): a RuntimeError, for the miniexpr cases with the prefilter's message. On main: 3 deadlocks (timeout), 6 segfaults, 1 SystemError, 1 ValueError (one thread with the GIL, where main raises the prefilter's error directly), and matmul with one thread and the GIL passes. The legacy cases are skipped on NumPy < 2 and WASM, where the legacy evaluator does not use the miniexpr prefilter.
    • test_prefilter_skips_python_callback_operands (Python filter on every route, Python codec on two; 1 and 4 threads): exact results. On main: 14 of the 16 cases fail (10 segfaults, 4 errors); only the single-thread typed compute() and [:] cases pass.
  • tests/ndarray/test_linalg.py: test_matmul_fast_path_skips_float_dtypes_without_kernel (float16, and longdouble where it is wider than float64: chunked path, exact results; fails on main), test_set_pref_matmul_rejects_dtypes_without_kernel (complex, bool, int16, float16, structured, mixed, non-native byte order: TypeError and no prefilter left set; fails on main), test_matmul_prefilter_supported_dtypes (float32, float64, int32, int64 through the prefilter, 1 and 4 threads, exact results; passes on main too).
  • Full suite: same results as main (only the 2 b2view tests that need aiohttp fail locally); no new compiler warnings (GCC 15, -Wall -Wextra).

Performance: no measurable change. blosc2.matmul() through the fast path (float32/float64, 512² to 2048²), (x * y + x).compute() and .sum() with the typed evaluator (1-D, 1e7 and 4e7 items: Menudet prefilter and native graph routes) and with the legacy one (2000² and 4000², miniexpr prefilter), 1 and 8 threads, A/B against main and main built a second time, alternating, 6 rounds on a 2x EPYC 9554 node pinned to 16 cores, wheels built with GCC 15: fix/main geomean 0.996 (main vs main 0.997), every cell within the main-vs-main spread except the 4e7-item typed sum() with 8 threads at +1.8%. That cell runs code this PR does not change: it is +1.8% also with the Python routing gate removed, and 0.994 with GCC 13 builds of the same code, so it is code placement.

AI disclosure: I used Claude Code (Anthropic) to find the problems, write the fix and the tests, run the benchmarks and draft this description. I reviewed and tested everything.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DiPFztdpnKMJkzrjT36BuP

C-Blosc2 runs prefilters on its worker threads when nthreads > 1 (and on the
calling thread without the GIL when compressions release it), but the error
paths of the miniexpr and matmul prefilters raised Python exceptions: Cython
took the GIL for the raise and the wrapper then called PyErr_Occurred()
without it.  With the caller holding the GIL, any error (e.g. an operand
chunk that fails to decompress) deadlocked matmul and legacy-evaluator
expressions and reductions with nthreads > 1; without it, it segfaulted.

Declare miniexpr_prefilter, matmul_prefilter and their helpers
`noexcept nogil` and remove every `raise` (Cython would still wrap one in
an implicit `with gil`); use cdivision there and in the Menudet prefilter
(the zero-division checks took the GIL as well).  aux_miniexpr() records
its error in its worker slot and returns -1; _store_data() and
_prefilter_data() raise it (C-Blosc2 ignores the return code of a
prefilter with a disposable output, i.e. of reductions).  aux_matmul()
frees what it allocated and returns -1.

These prefilters, and the workers of parallel native graph maps, decode
their inputs themselves, so operands with a codec or filter registered
from Python ran that callback without the GIL (segfault; the Menudet
reduction even with one thread).  The routing gates (matmul fast path,
_set_pref_expr(), _set_pref_matmul(), Menudet input eligibility, native
graph layout) now decline such operands, so they take another path.

matmul_prefilter raised "Unsupported dtype" for dtypes other than float and
int; blosc2.matmul() never sent those, but it sent float16 and longdouble
(kind "f"), which the kernels computed as float64: wrong results, and for
float16 reads and writes past the block buffers.  The fast path now takes
float32/float64 only, and _set_pref_matmul() checks the dtypes before the
prefilter is set.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DiPFztdpnKMJkzrjT36BuP

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant