Skip to content

perf: lazy-init _callbacks/_errbacks in ResponseFuture (25ns saving, 2x speedup, 112 bytes saved) - #795

Draft
mykaul wants to merge 1 commit into
scylladb:masterfrom
mykaul:perf/lazy-init-callbacks
Draft

perf: lazy-init _callbacks/_errbacks in ResponseFuture (25ns saving, 2x speedup, 112 bytes saved)#795
mykaul wants to merge 1 commit into
scylladb:masterfrom
mykaul:perf/lazy-init-callbacks

Conversation

@mykaul

@mykaul mykaul commented Apr 5, 2026

Copy link
Copy Markdown

Summary

  • Defer list allocation for _callbacks and _errbacks from ResponseFuture.__init__() to first use in add_callback()/add_errback()
  • On the synchronous execute path (session.execute()), no callbacks are registered, so both lists are never allocated — saving 112 bytes per request
  • All access is under _callback_lock; _set_final_result and _set_final_exception use or () guard to iterate safely when None

Benchmark

Scenario Per-call Speedup
Init with [] x2 50 ns baseline
Init with None x2 25 ns ~2x

Memory saved per request (no callbacks): 112 bytes (2 x 56-byte empty list)

Tests

  • 11 focused unit tests covering lazy init, callback/errback invocation, clear_callbacks, add_callback after result
  • Full unit test suite passes (656 passed)

@mykaul
mykaul force-pushed the perf/lazy-init-callbacks branch 2 times, most recently from 40c469e to 89dabcf Compare April 5, 2026 17:31
@mykaul mykaul changed the title perf: lazy-init _callbacks/_errbacks in ResponseFuture perf: lazy-init _callbacks/_errbacks in ResponseFuture (25ns saving, 2x speedup, 112 bytes saved) Apr 7, 2026
Defer list allocation for _callbacks and _errbacks from __init__ to
first use in add_callback()/add_errback(). On the synchronous execute
path (session.execute()), no callbacks are registered, so both lists
are never allocated — saving 112 bytes per request.

All access is under _callback_lock; _set_final_result and
_set_final_exception use 'or ()' guard to iterate safely when None.

Benchmark: 2.2x faster init (0.06 -> 0.03 us), 112 bytes saved/request.
Copilot AI review requested due to automatic review settings July 29, 2026 20:32
@mykaul
mykaul force-pushed the perf/lazy-init-callbacks branch from 89dabcf to 68ef1ef Compare July 29, 2026 20:32
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 25ecf7e0-8faf-479a-b3be-152055ad125c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@mykaul

mykaul commented Jul 29, 2026

Copy link
Copy Markdown
Author

Rebased onto current origin/master (past the bound_result_metadata / SCYLLA_USE_METADATA_ID changes in ResponseFuture.__init__). The rebase applied cleanly with no manual conflict resolution needed — the lazy-init hunks (class body, __init__, _set_final_result/_set_final_exception guards, add_callback/add_errback lazy-alloc, clear_callbacks) landed in the surrounding new code without overlap.

Given the recent history of ResponseFuture.__init__ collisions on other PRs (#806, #805), I specifically audited every direct ._callbacks/._errbacks access across the whole codebase (not just add_callback/add_errback) to make sure nothing reads them as a list/tuple before they'd be lazily created:

  • All access in cassandra/cluster.py is already properly guarded (is None checks before append, or () guards in the two iteration sites, clear_callbacks resets to None).
  • The only other ._callbacks in the repo is cqlengine's unrelated BatchQuery._callbacks, which is a separate class, always initializes to [], and is untouched by this change.
  • No getattr/hasattr indirection or test mocks touch ResponseFuture._callbacks/._errbacks directly outside of this PR's own add_callback/add_errback/_set_final_result/_set_final_exception paths.

No unresolved review threads or comments existed on the PR prior to this push.

Verified:

  • tests/unit/test_response_future.py + tests/unit/test_lazy_init_callbacks.py: 63 passed
  • Full tests/unit/: 731 passed, 88 skipped (environment-gated), 0 failed

Force-pushed the rebased commit (same commit, no content changes needed beyond the rebase) to keep this a single amended commit rather than adding new ones. Still a draft.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ResponseFuture by lazily initializing _callbacks/_errbacks to avoid unnecessary list allocations on the synchronous execute path, while keeping callback behavior consistent and thread-safe.

Changes:

  • Change _callbacks/_errbacks initialization from empty lists to None, and lazily allocate on first add_* use
  • Guard callback/errback iteration in _set_final_result / _set_final_exception to handle None safely
  • Add unit tests and a micro-benchmark to validate and measure the optimization

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
cassandra/cluster.py Implements lazy init and safe iteration for callbacks/errbacks in ResponseFuture.
tests/unit/test_lazy_init_callbacks.py Adds focused unit tests validating lazy init and callback/errback behavior.
benchmarks/bench_lazy_init_callbacks.py Adds a micro-benchmark to quantify time and allocation savings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +4 to +8
import unittest
from unittest.mock import Mock, patch, PropertyMock
from threading import Lock, Event

from cassandra.cluster import ResponseFuture, _NOT_SET
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants