-
-
Notifications
You must be signed in to change notification settings - Fork 104
perf: cache signature binding results #550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: cache signature binding results #550
Conversation
66bc139 to
17e1b35
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #550 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 25
Lines 1631 1656 +25
Branches 257 204 -53
=========================================
+ Hits 1631 1656 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The `signature_adapter` causes significant overhead in hot transition paths due to repeated signature binding on every callback invocation. This PR implements caching for `bind_expected()` to avoid recomputing argument bindings when the kwargs pattern is unchanged. ## Root Cause The `SignatureAdapter.bind_expected()` method iterates through all parameters and matches them against the provided kwargs on every invocation. In typical state machine usage, callbacks are invoked repeatedly with the same kwargs keys (e.g., `source`, `target`, `event`), making this repeated computation wasteful. ## Fix Added a per-instance cache (`_bind_cache`) to `SignatureAdapter` that stores "binding templates" based on the arguments structure: - **Cache key**: `(len(args), frozenset(kwargs.keys()))` - **Cache value**: A template of which parameters to extract - **Fast path**: On cache hit, extract arguments directly using the template (~1 µs) - **Slow path**: First call computes full binding and stores template (~2 µs) This approach preserves **full Dependency Injection functionality** - callbacks still receive correctly filtered arguments (`source`, `target`, etc.). ## Performance When measuring `bind_expected()` in isolation: - **Cached**: 0.86 µs/call - **Uncached**: 2.12 µs/call - **Improvement**: ~59% This is consistent with the ~30% end-to-end improvement reported in fgmacedo#548, as binding is one of several components in a full transition. ## Testing All existing tests pass (328 passed, 9 xfailed). Fixes fgmacedo#548
17e1b35 to
277271d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rodrigobnogueira, great work on this PR! The caching approach is solid and the performance gains are clear. I found one correctness issue that needs to be fixed before merging.
Bug: _fast_bind leaks named params into **kwargs
When a callback has both named parameters and **kwargs, the fast path includes all kwargs keys in the **kwargs dict, instead of only the unmatched ones.
Root cause: _full_bind mutates the kwargs dict via .pop(), so by the time it assigns to the **kwargs parameter, only unmatched keys remain. But _fast_bind uses .get() (doesn't mutate), so the full original dict is passed through.
# _fast_bind — kwargs is never filtered:
if kwargs_param_name is not None:
arguments[kwargs_param_name] = kwargs # BUG: includes keys already assigned to named paramsProposed fix
if kwargs_param_name is not None:
matched = set(param_names)
arguments[kwargs_param_name] = {k: v for k, v in kwargs.items() if k not in matched}Regression test
I verified this locally, and the first test fails on this branch (proving the bug), and the second passes because the positional arg doesn't appear in the kwargs dict:
FAILED test_named_param_not_leaked_into_kwargs - TypeError: named_and_kwargs() got multiple values for argument 'source'
PASSED test_kwargs_only_receives_unmatched_keys_with_positional
The failure shows the _fast_bind producing BoundArguments (source='X', kwargs={'source': 'X', 'target': 'Y', 'event': 'stop'}) — source is duplicated in **kwargs.
Here are the tests:
# I've added to tests/test_signature.py
def named_and_kwargs(source, **kwargs):
return source, kwargs
class TestCachedBindExpected:
"""Tests that exercise the cache fast-path by calling the same wrapped
function twice with the same argument shape."""
def setup_method(self):
SignatureAdapter.from_callable.clear_cache()
def test_named_param_not_leaked_into_kwargs(self):
"""Named params should not appear in the **kwargs dict on cache hit."""
wrapped = callable_method(named_and_kwargs)
# 1st call: cache miss -> _full_bind
result1 = wrapped(source="A", target="B", event="go")
assert result1 == ("A", {"target": "B", "event": "go"})
# 2nd call: cache hit -> _fast_bind
result2 = wrapped(source="X", target="Y", event="stop")
assert result2 == ("X", {"target": "Y", "event": "stop"})
def test_kwargs_only_receives_unmatched_keys_with_positional(self):
"""When mixing positional and keyword args with **kwargs."""
wrapped = callable_method(named_and_kwargs)
result1 = wrapped("A", target="B")
assert result1 == ("A", {"target": "B"})
result2 = wrapped("X", target="Y")
assert result2 == ("X", {"target": "Y"})Everything else looks good, the template approach, cache key design, and integration with the existing signature_cache are all well done. Just needs the **kwargs filtering fix and a test to cover it. 👍
…ture binding by filtering out named parameters.
|
Hello @fgmacedo , great catch! Bug fix in signature.py: Thanks 🙏 |
|



Description
The
signature_adaptercauses significant overhead in hot transition paths due to repeated signature binding on every callback invocation. This PR implements caching forbind_expected()to avoid recomputing argument bindings when the kwargs pattern is unchanged.Followed the ideas from @ojomio and @fgmacedo as discussed in #548.
Root Cause
The
SignatureAdapter.bind_expected()method iterates through all parameters and matches them against the provided kwargs on every invocation. In typical state machine usage, callbacks are invoked repeatedly with the same kwargs keys (e.g.,source,target,event), making this repeated computation wasteful.Fix
Added a per-instance cache (
_bind_cache) toSignatureAdapterthat stores "binding templates" based on the arguments structure:(len(args), frozenset(kwargs.keys()))This approach preserves full Dependency Injection functionality - callbacks still receive correctly filtered arguments (
source,target, etc.).Performance
When measuring
bind_expected()in isolation:This is consistent with the ~30% end-to-end improvement reported in #548, as binding is one of several components in a full transition.
Testing
All existing tests pass (328 passed, 9 xfailed).
Fixes #548