Numba: rework the vectorize intrinsic interface to fix wide-node compile blowup - #2354
Numba: rework the vectorize intrinsic interface to fix wide-node compile blowup#2354velochy wants to merge 2 commits into
Conversation
|
See #1971 although I found it to be a wash in the pymc-model catalogue, although the motivating issue shows it can matter. |
| # --- Wide-arity splitting (numba) ------------------------------------------- | ||
| # Calls (and tuple displays) with >30 items compile to LIST_APPEND + | ||
| # CALL_FUNCTION_EX bytecode, which numba lowers quadratically in the argument | ||
| # count. Split wide associative Elemwise and Join nodes into balanced trees so | ||
| # that no call — including the idx arrays gather-fusion may later add to an | ||
| # elemwise — crosses that threshold. Small chunks are deliberate: sibling tree | ||
| # nodes share a numba signature, so they compile once and hit the cache. |
There was a problem hiding this comment.
please no global level comments like this.
There was a problem hiding this comment.
Folded into the rewrite docstrings; the registration position is now explained at the optdb.register call instead.
| from pytensor.tensor.basic import Join, join | ||
|
|
||
| if not isinstance(node.op, Join): | ||
| return None |
There was a problem hiding this comment.
dumbest ever rewrite code I've seen. why is Join not in tracks?
There was a problem hiding this comment.
Fixed — tracks=[Join]. The None tracking was left over from an earlier iteration, no excuse.
| if len(tensors) <= _JOIN_SPLIT_THRESHOLD: | ||
| return None | ||
| axis = node.op.axis | ||
| new_out = _tree_reduce(lambda items: join(axis, *items), tensors, _JOIN_CHUNK) |
There was a problem hiding this comment.
similar to the wanting to associated add/mul correctly, you also want to think how you associated join, as every join require a full copy of all the inputs. You can easily create bad graphs where you are copying 90% of the contents multiple times to add a scalar at a time. If anything I'd suggesting thinking about a serial set_subtensor rewrite on empty. See #2014 which was more interested in reducing allocations/perfromance, not on what you're at here obviously
There was a problem hiding this comment.
You're right that the tree re-copies earlier chunks at every level. Replaced with your suggestion: serial set_subtensor into one preallocated buffer — each input written exactly once, and the chain collapses to inplace updates of a single allocation after the inplace pass (a 50-input join compiles to one AllocEmpty + 50 inplace IncSubtensor). It also removes the wide-tuple np.concatenate callee, which was the other half of the Join cost. #2014's allocation-reduction angle would compose with this but isn't needed for the compile-time goal here.
| "split_wide_add_mul", | ||
| dfs_rewriter(split_wide_add_mul), | ||
| "numba", | ||
| # after AddDestroyHandler (49.5), before inplace_elemwise (50.5) |
There was a problem hiding this comment.
It must run after the last Add-canonicalization — local_add_canonizer re-flattens the tree, so registering earlier ping-pongs — and before inplace_elemwise (50.5) so the tree/chain nodes get inplace treatment; the gather-fusion db (100) then only sees narrow nodes. Now stated at the registration site. The "after AddDestroyHandler (49.5)" phrasing in the old comment was incidental, not a dependency.
4683584 to
2dbb29e
Compare
| ) | ||
|
|
||
|
|
||
| # Numba types/lowers an op implementation that receives its inputs as one wide |
There was a problem hiding this comment.
#2361 fixes only the call sites (the caller-side prefix-tuple chain). The callee cost is separate: an implementation typed with a wide tuple (np.concatenate's
tuple argument, fused_elemwise_fn's star-args) lowers each element access with the full tuple type spelled out, so it stays quadratic with #2361 alone. Measured on the n=80 additive repro (nutpie, cold caches): main 480 s / 2.6 GB, #2361 alone 360 s / 1.7 GB, this PR alone 83 s / 0.97 GB, both 67 s / 0.97 GB — the two attack different quadratics and this one carries most of the win.
On Join you're right that a dispatch-level fix is possible: a generated per-arity impl with individual parameters (the #2310 style) would be linear with the same copy count, and would avoid the node-count blowup the serial set_subtensor chain causes (~2× nodes on graphs with a wide join). The rewrite's advantage is only that it reuses the existing inplace machinery instead of adding codegen. Happy to replace the Join rewrite with a numba dispatch along the #2310 lines if you prefer that shape — the Add/Mul split can't move to dispatch the same way, since it's the fused kernel's own signature that must stay narrow.
There was a problem hiding this comment.
Join is a better representation then nested set subtensor and the inplace doesn't matter. In either case you allocated an empty at the start and then write the values without copying intermediates. A custom join dispatch would never do this while nested set subtensor is at the mercy of the optimizer to get right.
The add/mul split I'm not sold on that yet. In fact we want to flatten more associative commutative Ops like And, Or...
You're also targeting this but I don't see what's specific about it. Fusion would gladly take the same number of args on a Composite and the problem would be the same. What's unique about add or mul other than it showed in your model?
If you want to split it should be in a form that has at least computational motivation like the PR I mentioned.
Otherwise if it's really the scalar add or mul you can tweak the dispatch. Right now it does (a + b + ... c + d), you can consider (a + b + ...) + (... + c + d) if it helps or even a kahman pairing or something like that as it doesn't change the meaning of the scalar add.
|
Reworked per review: the graph rewrites are gone, everything is now at the dispatch layer, and it turned out better than the rewrites on every measurement. Two smaller pieces are split off for separate review: #2362 (Join dispatch with named parameters and slice writes — your suggestion) and #2363 (pairwise Add/Mul — also your suggestion). This PR keeps the core piece: the vectorize intrinsic interface rework. #2361 remains the call-site companion. Two findings worth flagging from the profiling that got here. First, splitting wide Add/Mul turned out unnecessary: after fixing the boundaries, the residual cost was not LLVM on big kernels but numba type inference re-hashing and re-unpickling the encoded-literal metadata arguments on every fixpoint sweep — moving the metadata into per-node intrinsic closures fixed it, and dispatch-only now beats the old rewrite branch (n=80 repro: 77 s / 0.84 GB vs 67 s / 0.97 GB; nutpie#339 sentinel lognormal: 58 s / 0.70 GB vs 61 s / 0.77 GB; main: 480 s / 2.6 GB and 200 s / 3.6 GB). Second, the generic caching wrapper in dispatch/basic.py wrapped every funcified op in |
ricardoV94
left a comment
There was a problem hiding this comment.
One can't avoid string codegen in numba no matter how hard we try hein
| return jitable_func, None | ||
| else: | ||
| op_name = jitable_func.__name__ | ||
| # Named parameters, not *args: numba types a star-args wrapper with one |
There was a problem hiding this comment.
Are you sure about this? Doesn't numba just passthrough without deconstructing anything? Does it depend on numba version? And above 30 arguments wouldn't matter either way right? Because it still triggers CPython tuple thing?
There was a problem hiding this comment.
You were right — dropped, basic.py is no longer touched by this PR.
Measurements, since the questions are worth answering properly. Pass-through: at runtime yes, nothing survives — post-optimization IR is 0.884 MB (star) vs 0.910 MB (named) for an 80-argument call chain. At compile time no: the caller has to emit the tuple (an insertvalue chain, 132 KB of IR) and the wrapper to unpack it (extractvalue, 115 KB), so pre-opt IR is 3.39 MB vs 2.67 MB — LLVM parses and then deletes it. Version: not version-dependent, byte-identical IR sizes on numba 0.58.1, 0.61.2 and 0.65.1. >30 arguments: exactly your point, and it cuts deeper than 'no difference' — the wrapper's own call re-triggers the CPython path, so unpatched the named form is worse (5.50 MB vs 3.64 MB at 40 args, paying the chain twice instead of once); it only comes out ahead with #2361 (2.67 vs 3.39 MB at 80).
And on the real model none of it matters: interleaved A/B/A/B on the n=80 repro, cold caches, named 106.9 / 106.5 s vs star 106.9 / 106.7 s, RSS within 1.5%. A 21% pre-opt IR reduction on one wrapper doesn't survive to whole-model compile time, so the special case wasn't earning the kwargs.get("node") branch.
There was a problem hiding this comment.
Real Margus here: I'm re-running on my real models to measure the diff with them, as AI's comment on "on the real model" still talks about a toy example. But yes, if that also comes out a wash, it makes sense to close this PR as 2361+2362 do all of the real work.
Two dispatch-level costs made >30-input nodes compile quadratically. Star-args boundaries type as one wide tuple whose LLVM lowering is O(n^2) in the arity: the _vectorized intrinsic and the Elemwise and FusedElemwise overload targets and impls now use named parameters, with the outer inputs as trailing individual arguments of a per-node intrinsic instance. And the per-node metadata was passed as base64-pickled literal string arguments, which numba's type inference re-hashes and re-decodes on every fixpoint sweep — for wide nodes this dominated compile time outright; the intrinsic instances now close over the decoded metadata instead, leaving only runtime values in the typed signature. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Also corrected the Results section: the earlier '77 s / 0.84 GB' figure did not reproduce. This box is single-core and shared with a desktop session, so absolute compile times move with ambient load, and that number came from a different session than the The A/B ablations quoted in this thread were all interleaved within a session, so they are unaffected. |
|
I thought this PR was trying an alternative to #2361 that didn't require the hacking of numba |
No not really:
Also: I understood you were mostly ok with #2361 at least until they merge the fix themselves? |
|
I'm not ok with #2361, not against it either, rather apprehensive. Would really like to get a positive reaction from the numba guys first. It touches internals of numba, including outside of pytensor use. |
|
But now I'm also confused about #2362, the argument names thing is the same thing you were doing here no? If it doesn't help here why does it help there? You sure the speedup is not just the slice assignemnt instead of the |
Problem
Nodes with more than ~30 inputs (wide fused kernels from additive models, wide scalar Composites from gradient fusion) compiled quadratically in numba, in both time and memory — a 62 GB / killed-by-OOM real-model report in pymc-devs/nutpie#339. Two dispatch-level mechanisms were responsible. Star-args boundaries type as one wide tuple, whose LLVM lowering is O(n²) in the arity: the
_vectorizedintrinsic's inputs tuple and theElemwise/FusedElemwiseoverload targets and impls. And the per-node metadata (broadcast patterns, dtypes, index specs) was passed to the intrinsic as base64-pickled literal-string arguments, which numba's type inference re-hashes and re-decodes on every fixpoint sweep — profiling showed this dominating compile time outright on wide graphs.Changes
All boundaries now use named parameters, with the outer inputs as trailing individual arguments of a per-node intrinsic instance that closes over its decoded metadata — only runtime values remain in typed signatures. Graphs are untouched. Pairs with #2361 (or numba/numba#10782 once released) for the >30-argument call sites in generated fgraph functions; without it results are unchanged but wide calls still pay the bytecode-chain cost.
Results
Interleaved with
mainin a single session on an otherwise-idle 1-core box, cold caches (absolute times on this box are load-sensitive, so only same-session interleaved comparisons are quoted; measured together with #2361 and #2362, since the wide-node bottlenecks compose and any single one left unfixed caps the win):That is 3.5× compile and 2.8× peak RSS on the repro, and on the sentinel it removes the pathology the issue is actually about: the lognormal/gamma disparity goes from 1.4× time / 3.1× memory to 1.0× / 0.9×. logp/grad parity exact on the sentinel; full numba test battery green with cold caches.