Conversation
cmdupuis3
added
run-benchmark
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
Benchmarks that have got worse:
|
dask="parallelized" on the threaded scheduler can only overlap work that releases the GIL, and only 2 of the package's 140 @njit sites passed nogil=True. Adds it to 19 more: the kernels that Python invokes once per array, which is exactly the set dask will call once per chunk. Measured on _construct_edge_node_distances (2M edges, best of 3), the same kernel jitted with and without nogil: threads GIL held nogil speedup 1 96.9ms 95.7ms 1.01x 2 192.8ms 103.8ms 1.86x 4 401.4ms 105.3ms 3.81x 8 780.7ms 119.5ms 6.53x The GIL-held column scales linearly with thread count -- that is the signature of zero parallelism. Deliberately NOT a blanket sweep. nogil is not free and not universally inert: * It only takes effect at the Python->native boundary. An njit->njit call never touches the GIL, so on the 106 kernels reachable only from other jitted code the flag is dead weight. That includes every inline="always" primitive in utils/computing.py, which by construction is inlined into its caller and has no boundary at all. * At that boundary it costs a PyEval_SaveThread/RestoreThread pair, measured here at ~40ns per call. Against a whole-array kernel that is free; against a scalar kernel called from a Python loop it is a 20-30% regression. An earlier revision of this commit did apply nogil everywhere and asv caught exactly that -- two_sum 1.26x, two_prod 1.21x, diff_of_products ~1.25x, acc_sqrt_re 1.37x, orient3d_on_sphere 1.17x, all of them sub-microsecond kernels. So the rule is per-call work, not per-decorator uniformity. Kernels left alone for that reason include gca_const_lat_intersection and get_number_of_intersections (per-edge, from the Python loops in zonal.py and integrate.py), _compute_band_overlap_area (per-face, zonal.py:327), _barycentric_coordinates (per-candidate, neighbors.py:954) and the EFT primitives above. Several of these are slated to become gufuncs; they can take nogil when they stop being called per element. Two further sites stay as they are: * the guvectorize kernel in grid/neighbors.py -- numba rejects nogil as a @guvectorize option, and the gufunc machinery already drops the GIL around the loop. * the 15 parallel=True kernels keep parallel=True. Nested under dask's threadpool it oversubscribes, but dropping it belongs with the work that gives dask the chunk loop. _build_node_edge_connectivity was the package's only bare @njit and so also its only kernel without cache=True; normalized to @njit(cache=True, nogil=True) while adding the flag. Test suite: 953 passed, 1 skipped. test_plot_with_features fails identically before and after (matplotlib figure size, unrelated). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cmdupuis3
force-pushed
the
cmd/nogil
branch
from
September 11, 2026 21:51
3c8d2df to
d2fcf31
Compare
The previous commit's flag placement is not self-evident from reading a
decorator, and nothing in the repo could catch it drifting. Both failure
directions are silent:
* dropping nogil from an entry point serializes it under dask's
threaded scheduler with no error, and no timing in the asv suite
moves, because every benchmark in it drives uxarray from one thread;
* adding nogil to a kernel only jitted code calls does nothing useful
-- its caller already released the GIL for the whole call tree -- but
still pays ~40ns per direct Python call, which is how the earlier
blanket sweep turned into six asv regressions.
test_nogil_policy.py states the rule as four assertions over an AST scan
of the package: Python-reachable njit kernels must release the GIL;
jit-only ones must not set the flag; inline="always" kernels must not
(they have no call boundary to release at, and the plan commits to
keeping them inlined); and gufuncs must not (numba rejects the option).
"Reachable" counts a bare reference, not just a call. The kernels that
matter most are passed to xr.apply_ufunc rather than called -- notably
_build_n_nodes_per_face at connectivity.py:136 -- and a call-only scan
would classify exactly those as jit-only, which is backwards.
Seven kernels are exempt, each with its reason recorded; a further test
fails if an exemption stops being live, so the list cannot rot as these
become gufuncs. Each assertion was mutation-tested: reverting the flag on
an entry point, adding it to a jit-only kernel, to an inline kernel, and
to an exempt kernel are all caught.
nogil_scaling.py measures wall(2 threads)/wall(1 thread) on
_construct_edge_node_distances, alongside the same kernel recompiled
without the flag as a control:
track_gil_scaling 1.07 (overlaps)
track_gil_scaling_control 1.97 (serializes)
This benchmarks the flag rather than a user-facing operation, which is a
fair criticism -- but no user-facing path reaches a kernel from two
threads until core/ grows the chunked call sites, so there is nothing
else to measure yet. Its docstring says to retire it for a real chunked
workload once one exists.
Test suite: 965 passed, 1 skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_nogil_policy.py parsed every module with ``path.read_text()`` and no
encoding, so it decoded with the locale's. On Linux and macOS that is
UTF-8 and the scan passed; on Windows it is cp1252, which has no mapping
for byte 0x90.
Four modules carry a U+2010 HYPHEN (core/dataarray.py, core/dataset.py,
cross_sections/dataarray_accessor.py, grid/point_in_face.py), encoded
E2 80 90 -- and that 0x90 is undefined in cp1252:
UnicodeDecodeError: 'charmap' codec can't decode byte 0x90 in
position 82386: character maps to <undefined>
The scan runs at module scope, so the failure aborted collection and
errored all 12 tests on every Windows job in the matrix. Python 3.11
through 3.14 all default to the locale encoding there, which matches the
failure appearing on the Windows builds alone.
Pass encoding="utf-8" explicitly. Verified with -X warn_default_encoding
and EncodingWarning promoted to an error, so no implicit-encoding read
remains in the file.
Test suite: 965 passed, 1 skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drops test_nogil_policy.py and the control arm of the scaling benchmark, leaving the branch at the nogil=True decorator lines plus a single track_gil_scaling number. The policy test encoded, as four AST assertions and seven documented exemptions, a rule that applies to 20 decorator lines. That is a lot of machinery for the size of the change, and the scan carried its own maintenance cost -- it already needed one fix for reading package sources with the locale encoding, which broke the Windows matrix. What is lost is the guard against the flag spreading back onto kernels that do not want it. The regression it mattered most for is still covered: dropping nogil from an entry point moves track_gil_scaling from ~1.07 toward ~2.0, which asv flags. Nothing now catches the other direction -- a blanket re-application -- except asv noticing the sub-microsecond kernels regress, which is how it was caught the first time. track_gil_scaling_control was the same kernel recompiled without the flag, there so a broken harness could not read as a good result. Without it the absolute value carries less weight; the regression signal does not depend on it. Test suite: 953 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cmdupuis3
marked this pull request as ready for review
September 21, 2026 22:59
Sevans711
approved these changes
Sep 22, 2026
Sevans711
left a comment
Sevans711
left a comment
Collaborator
There was a problem hiding this comment.
Looks ready to merge! Simple changes which should help with future scalability work, and shouldn't hurt in the meantime (no regressions in benchmarks). The new benchmark is a good spot check to help prevent regression, and demonstrates clearly that the nogil=True flag can cause improvements. Approving as-is!
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
2 participants
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.
Closes #1756
Overview
Only 2 of the 140 @njit sites in the package passed nogil=True (grid/connectivity.py, grid/area.py). Every other compiled kernel held the GIL for its whole execution, so calling them from dask's threaded scheduler as dask="parallelized" does serialized them completely, regardless of how many workers were configured.
Adds nogil=True to the 20 or so remaining top-level sites making @njit(cache=True, nogil=True) the default pattern. These are all nopython, allocation-light leaf kernels with no object-mode fallback.
Measured on _construct_edge_node_distances (2M edges, best of 3), the same kernel jitted with and without nogil:
The GIL-held column scales linearly with thread count -- that is the signature of zero parallelism.
Two sites are deliberately left alone:
_build_node_edge_connectivity in grid/connectivity.py is also the one kernel still missing cache=True; left as-is to keep this diff to nogil.
The
nogil_scaling.GILScaling.track_gil_scalingis added to prove thatnogilis working as expected.PR Checklist
General
Testing & Benchmarking
AI Disclosure
AI Usage: Claude Opus 5