perf(promotion): skip conversion when a return annotation is already satisfied - #292
perf(promotion): skip conversion when a return annotation is already satisfied#292nstarman wants to merge 4 commits into
Conversation
…satisfied
A method with a return annotation pays `convert` on every call. `convert` walks the
annotation with `resolve_type_hint` twice, dispatches to find a conversion method,
builds an `_InvokedMethod` wrapper via `invoke`, and runs a `beartype` check -- almost
always to hand back the object it was given, because a return annotation is
overwhelmingly a type the method already returns.
Memoise that. `plum._function.identity_conversions` records, per
`(type(obj), target_type)`, whether conversion is the identity, and `_convert` returns
the object directly on a hit. Two conditions make the answer a property of the key
rather than of the object, so that the memo cannot change a result:
* the conversion resolved to the fallback method, i.e. no conversion method applied;
* `is_faithful(resolved)`, defined as `isinstance(x, t) == issubclass(type(x), t)`,
so the fallback's check depends only on `type(obj)`. Without it the match can
depend on the value -- a `Literal`, a custom `__instancecheck__` -- and is re-run.
Negative answers are recorded too, so the analysis (which includes an uncached
`is_faithful` walk) runs once per pair rather than once per call; without that,
`convert` would get slower for every pair the memo cannot help.
Invalidation mirrors the existing method cache: `clear_all_cache` clears it, and so
does `add_conversion_method`, since a newly registered method can claim a pair that was
previously an identity. The same in-place type-mutation caveat as `Function._cache`
applies, and is already documented.
`convert` also now calls the resolved method directly instead of through `invoke`,
which built a wrapper object per call.
tests/benchmark.py, medians (every pre-existing benchmark unchanged):
| Path | Before | After | Gain |
|---------------------------|---------|---------|-----------|
| Annotated return | 3.68 us | 0.51 us | **7.2x** |
| Annotated return, union | 7.03 us | 0.50 us | **14.1x** |
Every function in the benchmark suite returned through an unannotated method, which
`_convert` already short-circuits, so this cost was invisible; a benchmark for
annotated returns is added.
Coverage Report for CI Build 30662553638Coverage increased (+0.009%) to 99.503%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR optimizes Plum’s return-annotation conversion path by memoizing when convert is effectively an identity for a given (type(obj), target_type) pair, allowing dispatched calls to skip expensive conversion/type-walking work on subsequent calls. It also reduces per-call overhead by invoking the resolved conversion method directly (avoiding wrapper allocation via invoke).
Changes:
- Add a global identity-conversion memo (
plum._function.identity_conversions) and fast-path inplum._function._convertto bypassplum.convertwhen a return annotation is known-satisfied. - Update
plum.convertto consult/populate the memo (including negative caching) and to call resolved conversion methods directly. - Add tests validating memo soundness/invalidation and extend benchmarks to cover annotated-return performance.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_promotion.py | Adds tests that pin correctness conditions for memoizing/skipping identity conversions on annotated returns. |
| tests/conftest.py | Ensures identity-conversion memo is cleared when conversion methods are restored between tests. |
| tests/benchmark.py | Adds benchmarks specifically measuring annotated-return and union-return overhead. |
| src/plum/_promotion.py | Implements memo lookup/recording in convert, adds size limit, and clears memo on add_conversion_method. |
| src/plum/_function.py | Introduces identity_conversions storage plus a fast-path in _convert to skip convert when memoized. |
| src/plum/_dispatcher.py | Extends clear_all_cache() to also clear the identity-conversion memo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The two `except TypeError` branches around the memo lookup, and the `suppress` around the store, were unreachable and showed up as the only uncovered lines in the diff. An unhashable `type_to` has never been usable: `convert` hashes it to look up the conversion method (`Function._cache`), so `plum.convert(obj, [int])` raises `TypeError: unhashable type: 'list'` on `master` already. Catching the error around the memo lookup only moved where the identical `TypeError` came from, so the guards protected nothing and the `suppress` around the store could never fire -- the store is reached only when the lookup has already hashed successfully. Removing them restores the coverage profile to `master`'s exactly (`_promotion` and `_dispatcher` at 100%, `_function` at 99% with the same single pre-existing partial branch) and takes a `try` off the hot path. A test pins the unhashable-target behaviour instead, since this change is what introduced the extra hashing and the equivalence is the thing worth guarding.
`_clear_identity_conversions()` only wrapped `identity_conversions.clear()`, so callers now clear the dict themselves. Verified that the indirection was not buying anything across the module boundary: a `mypyc`-compiled `_dispatcher` importing the name gets the same object that `_function` reads and `_promotion` populates, both when `_function` is interpreted (this branch) and when it is natively compiled (as under beartype#288), so `clear_all_cache()` empties the live memo in either build.
The comments were longer than the code they explained, and most of it restated what the code says. Kept only what is not derivable from reading it: why both conditions are needed for the recorded answer to be a property of the key, why negatives are recorded as well, and the staleness contract. -83 lines of prose, no behaviour change.
|
Ping @wesselb, I think this PR is ready. |
|
@wesselb Should I add a speed benchmark to prevent future regressions? |
Summary
A method with a return annotation pays
converton every call.convertwalks the annotation withresolve_type_hinttwice, dispatches to find a conversion method, builds an_InvokedMethodwrapper viainvoke, and runs abeartypecheck — almost always to hand back the object it was given, because a return annotation is overwhelmingly a type the method already returns.tests/benchmark.py, medians (every pre-existing benchmark unchanged):For scale: an unannotated return costs ~0.43 µs (7× native). Annotating the return made the same call 57×–109× native. After this change it is 8×.
Found while profiling unxt, where
u.unit(...)— a three-method dispatch whose bodies are a dict lookup or an identity — cost ~9 µs a call. ~95% of that was this path;resolve_type_hintalone was 64% of cumulative time under cProfile, called 10× perunit()call.How
plum._function.identity_conversionsrecords, per(type(obj), target_type), whether conversion is the identity;_convertreturns the object directly on a hit.Two conditions make the answer a property of the key rather than of the object, which is what makes the memo unable to change a result:
is_faithful(resolved)— defined in plum asisinstance(x, t) == issubclass(type(x), t), so the fallback's check depends only ontype(obj). Without it the match can depend on the value (aLiteral, a custom__instancecheck__) and must be re-run.This is the same invariant
Function._cachealready relies on to key methods on argument types, applied to the return side.Negative answers are recorded too. That matters:
is_faithfulis uncached and costs ~6 µs on a 4-way union, so without recording negativesconvertwould get slower for every pair the memo cannot help. With them, the analysis runs once per pair rather than once per call — the paths the memo can't short-circuit still come out ahead:-> Literal[1](unfaithful, never memoised)-> Basewith a conversion method registered-> Any(control, already short-circuited)convertalso now calls the resolved method directly instead of throughinvoke, which built a wrapper object per call — that is where the non-memoised gains come from.Invalidation
Mirrors the existing method cache:
clear_all_cache()clears it, and so doesadd_conversion_method, since a newly registered method can claim a pair that was previously an identity (type_frommay be a subclass oftype_to). The same in-place type-mutation caveat asFunction._cacheapplies —type_mapping, a later-deliveredModuleType— and is already documented as requiringclear_all_cache.Testing
tests/test_promotion.pypinning the soundness conditions, not the speed. They go through a dispatched function, because that — notplum.convert— is what reads the memo.is_faithfulgate, either invalidation hook, the fallback-method check, or the size bound each makes a specific test fail.__instancecheck__judges differently;Literaltargets; a raising conversion (never recorded); union targets; the size bound.mypy --enable-error-code=no-redef,pyright, andruffclean.Compatibility
_functionnatively for faster dispatch #288 (mypyc-nativeFunction): verified — built the compiled wheel with these changes on top of a tree containing build(mypyc): compile_functionnatively for faster dispatch #288,assert COMPILEDpasses,_functionis a native.so, and the mypyc CI test selection is green. The new module state is a plain annotateddictplus adef, mirroring the existing_owner_transfer. The size-limit constant deliberately lives in the uncompiled_promotionso it stays monkeypatchable under a compiled build.feat/cacheability-aspects: orthogonal — that work caches the argument side, this one the return side, which it leaves untouched (measured at 8.8 µs for a union return both before and after it). Applies cleanly on top; full suite and the new tests pass there too.u.unit(apyu.km)goes 9.6 µs → 0.55 µs.🤖 Generated with Claude Code