PERF: Skip Signature.bind() for the all-positional cells call - #269
Merged
Merged
Conversation
get_node() built every trace key with inspect.Signature.bind() plus BoundArguments.apply_defaults(), and the executor consults the value cache only after the key exists. A cache hit therefore paid a full bind(): over a sweep of the 28 lifelib-products models, 15.9M binds against 5.4M formula evaluations, with about two thirds of the binds serving calls that were already cached. Formula now derives _bind_tails from its signature once, at construction: _bind_tails[n] is the tuple of defaults to append to n positional arguments to reproduce the key bind() would produce, or None when n arguments would raise. It is None as a whole for any signature carrying a VAR_POSITIONAL, KEYWORD_ONLY or VAR_KEYWORD parameter, which bind() canonicalises in ways the shortcut cannot. get_node() consults it for the all-positional call and falls through to the untouched _bind_args() otherwise, so rejected calls raise the very same TypeError with the very same message. Every guard clause is load-bearing: * args.__class__ is tuple - CellsImpl.find_match passes a *list*, and returning it unchanged makes an unhashable cache key. * the parameter-kind check - without it f(1, 2) and f((1, 2)) on def f(*args) collapse onto one cache entry and silently alias, and a positional call to a keyword-only parameter becomes a valid entry instead of a TypeError. * the arity and tail checks - reject too many and too few arguments respectively, both by falling through to bind(). Because a stale _bind_tails is a wrong cache key rather than a crash, every assignment to Formula.signature is funnelled through _set_signature(). That also fixes _copy_other(), which iterated self.__slots__ and so copied nothing at all for ParamFunc, whose __slots__ = () shadows the base tuple: space.set_formula(NULL_FORMULA) used to yield a ParamFunc with no attributes set. Formula.parameters now reads the tuple cached by _set_signature rather than rebuilding it; that is tidiness, not a speedup - it is not read on the hot path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_positional_only asserted on "positional-only", the wording Signature.bind uses from Python 3.12 on. Python 3.9 to 3.11 say "'x' parameter is positional only, but was passed as a keyword" instead, so the test failed on those three rows of the CI matrix while the library behaved identically on all of them. Match "positional[- ]only", which covers both, and pin the real invariant alongside it: the message is exactly the one _bind_args raises. The keyword call never enters the fast path, so this is bind()'s own error either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fumitoh
added a commit
that referenced
this pull request
Aug 25, 2026
The working instruction that PR #269 was implemented from, filed verbatim alongside IOSpecLiteralTask.md and CoreRefactorDesign.md. Renamed from BIND-FASTPATH-PLAN.md to match the directory's convention. It records what the implementation cannot: the measurements that motivated the change, the decision table behind the guard, the two traps that only adversarial review found (find_match passing a list, and ParamFunc.__slots__ shadowing the base tuple), and the approaches ruled out so they are not retried. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
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.
What
get_node()built every trace key withinspect.Signature.bind()plusBoundArguments.apply_defaults(), and the executor consults the value cacheonly after the key exists. A cache hit therefore paid a full
bind().Formulanow derives_bind_tailsfrom its signature once, at construction:_bind_tails[n]is the tuple of defaults to append tonpositional argumentsto reproduce the key
bind()would produce, orNonewhennarguments wouldraise. It is
Noneas a whole for any signature carrying aVAR_POSITIONAL,KEYWORD_ONLYorVAR_KEYWORDparameter, whichbind()canonicalises in waysthe shortcut cannot.
get_node()consults it for the all-positional call andfalls through to the untouched
_bind_args()otherwise, so rejected callsraise the very same
TypeErrorwith the very same message.Why
Over a sweep of the 28
lifelib-productsmodels (234 model points):15,914,995 bind calls, roughly two thirds of them serving calls whose value
was already cached.
Interleaved A/B, 6 runs, arm order flipped each rep, each arm a fresh
subprocess against a
git worktreeofmain:One distinct SHA-256 digest of every
result_cf()frame across all sixruns — the two arms produce byte-identical results. Honest quote is the
range, -20% to -30%; per-model deltas under ~3 s are noise and should not
be read as signal.
Notes for a reviewer
Every guard clause in the fast path is load-bearing. Each was removed in
turn and the new test module re-run:
args.__class__ is tupletest_cells.py::test_match)not kwargs_set_signaturetail is not Nonenargs < len(tails)Two of those deserve spelling out:
args.__class__ is tupleis correctness, not micro-optimisation.CellsImpl.find_match()callsget_value()with a list, at full arity, sothe guard would otherwise fire and hand back an unhashable cache key.
def f(*args),f(1, 2)andf((1, 2))would collapse onto one cache entryand the second call would silently return the first's value.
A stale
_bind_tailsis a wrong cache key rather than a crash, so everyassignment to
Formula.signatureis funnelled through_set_signature().That also fixes
_copy_other(), which iteratedself.__slots__and so copiednothing at all for
ParamFunc, whose__slots__ = ()shadows the basetuple —
space.set_formula(NULL_FORMULA)used to yield aParamFuncwith noattributes set at all.
The tail of defaults is computed by walking backwards from the last
parameter rather than breaking at the first default. Same result for every real
signature —
inspect.Signaturerejects a non-default parameter after adefaulted one — but it can never place
Parameter.emptyinto a tail.Formula.parametersnow reads the tuple cached by_set_signatureinstead ofrebuilding it. That is tidiness; no speedup is claimed for it — it is not
read on the hot path.
node_get_args()is deliberately left alone: it ran 234 times against those15.9M bind calls, once per ItemSpace creation.
Testing
modelx/tests: 1394 passed, 6 skipped, against a pre-change baseline of1218 passed, 6 skipped — the delta is the 176 new tests. Green both with
-p no:randomlyand with random ordering.modelx/tests/core/cells/test_bind_fastpath.pycovers the full decisiontable at the
Cellslevel, asserting the cache keys incells._impl.dataand not only the returned values: all-positional at exact arity, keyword and
out-of-order-keyword and mixed calls, defaults called short and full, a
keyword overriding a default,
POSITIONAL_ONLY,KEYWORD_ONLY,VAR_POSITIONALnon-aliasing,VAR_KEYWORD, too-few and too-many arguments,Cells.match()with its masked list, and a formula-less Space.shapes x 42 call shapes, plus real
CellsImplobjects,get_node(obj, args, kwargs)must equal(obj, _bind_args(obj, args, kwargs))— including the exception type and message when it raises.
(uslib 8,400,622 / uklib 1,486,414 / jplib 6,027,959) with 100% of them
taking the fast path: 0 rejected by the kind guard, by kwargs, by arity or
by the tuple guard.
Compatibility
No syntax newer than 3.7;
Parameter.POSITIONAL_ONLYexists on every versionin the CI matrix. The fast path never touches
BoundArguments.arguments, whosetype varies across versions.
🤖 Generated with Claude Code