perf: replace functools.partial with closure in _query callback (100-200ns improvement - 1.3-1.6x speedup) - #786
Conversation
8809c16 to
9c8f83f
Compare
9c8f83f to
c67ef30
Compare
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR optimizes the hot-path response callback in ResponseFuture._query() by replacing a per-request functools.partial(...) allocation with a closure that captures the required arguments via default parameters.
Changes:
- Replace
partial(self._set_result, host, connection, pool)with a closure capturinghost/connection/pooland_set_result. - Reduce callback creation + invocation overhead on every request (per PR benchmarks).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _set_result = self._set_result | ||
|
|
||
| def cb(response, _h=host, _c=connection, _p=pool, _sr=_set_result): | ||
| _sr(_h, _c, _p, response) |
Signed-off-by: Yaniv Michael Kaul <yaniv.kaul@scylladb.com>
c67ef30 to
a442ae5
Compare
Motivation
ResponseFuture._query()creates afunctools.partial(self._set_result, host, connection, pool)on every single query (line 4623). This is the primary response callback, executed for every request.partialhas non-trivial allocation overhead: it creates a newpartialC object, allocates a tuple for positional args, and invocation goes throughpartial.__call__indirection.Summary
partial(self._set_result, host, connection, pool)with a closure using default-argument capturepartialobject allocation and__call__indirection__defaults__tuple, making both creation and invocation cheaperBefore
After
Testing
tests/unit/test_cluster.pypasscb(response)callsself._set_result(host, connection, pool, response)identicallypartialuses in cluster.py (connection factory, watchers, reprepare, timeout rescheduling) are left unchanged as they are on cold/rare pathsBenchmarks
Isolated benchmark measuring callback creation + 1 invocation (5M iterations):
functools.partial(before)The default-argument closure was chosen as it has the best total cost (creation + invocation), since both happen exactly once per request.
Pre-review checklist