Skip to content

perf(handoff-report): read selection heads in one snapshot - #2232

Open
Inference1 wants to merge 1 commit into
oceanbase:masterfrom
Inference1:perf/handoff-report-batch-heads
Open

Inference1 wants to merge 1 commit into
oceanbase:masterfrom
Inference1:perf/handoff-report-batch-heads

Conversation

@Inference1

Copy link
Copy Markdown
Contributor

Which issue or RFC does this PR close?

No issue to close. Found while reviewing cross-Scope report reads.

Rationale for this change

HandoffReportApplication.get_report resolved each selected Scope's Handoff with latest() (head read plus revision load) and then re-read the revision, so a selection cost about seven statements per Scope. mode=all therefore grew linearly with the number of Scopes.

Measured against the real server with mode=all (statements counted at the engine level):

selected Scopes before after
2 10 5
5 31 11
17 115 35

What changes are included in this PR?

  • HandoffReadAdapter now exposes latest_many(scope_ids). RuntimeHandoffReadAdapter reads all selected heads with one query per 100 Scopes inside a consistent_snapshot transaction and loads each selected revision on the same connection.
  • The report keeps the same freeze semantics: every entry still points at the exact revision its head selected in that snapshot, and HandoffReportInconsistentError is still raised if the repository returns a different revision.
  • RelationalContexts exposes handoff_artifact_id so composition can build the adapter without reading a private attribute.
  • New guard test_handoff_report_statement_count_does_not_grow_with_scope_count: 16 created Scopes plus the default Scope must stay within 2 * selected + 3 statements.

Are there any user-facing changes?

No. The report payload is unchanged.

How was this change tested?

  • pytest tests/test_handoff_report.py tests/test_client.py tests/test_mcp.py -q -> 49 passed.
  • pytest tests/test_system_cli.py -k handoff -q -> 8 passed.
  • The new guard fails on the pre-fix code (assert 115 <= 37) and passes with the change (35 statements).
  • ruff check and ruff format --check pass on all changed files.

The report resolved each selected Scope's latest Handoff with a separate head
read and revision load, so a selection cost about seven statements per Scope.
Read all heads in one snapshot and load each selected revision on the same
connection; the report still freezes one exact revision per Scope.

Measured with mode=all: 17 selected Scopes take 35 statements instead of 115.

@Fengzdadi Fengzdadi left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, the problem is real and the change is well scoped. I reproduced it locally (SQLite, every Scope with a committed Handoff, median of 7 mode=all requests):

Scopes before this PR
50 353 statements / 190 ms 103 statements / 47 ms
300 2103 statements / 1284 ms 606 statements / 297 ms

So even without network round trips this is roughly a 4x latency win, and tests/test_handoff_report.py passes on the branch. I have no blocking concerns. A few optional notes:

  1. The guard test name does not match its assertion. test_handoff_report_statement_count_does_not_grow_with_scope_count asserts statements <= 2 * selected + 3, which still allows linear growth: each Scope with a Handoff still costs two statements (the artifact row and its lineage), because ArtifactRepository.get_many and _load_lineage_many are bound to a single scope_id. Either name it after the budget it enforces (for example ..._stays_within_two_statements_per_scope), or assert the difference between two sizes so the test protects the property its name claims.

  2. The HandoffReportInconsistentError check in latest_many can no longer fire. The revision is loaded by its exact key inside the same snapshot that selected it, and the head-to-artifact foreign key guarantees the row exists, so handoff.as_ref() != reference is always false. Consider dropping it, or adding a short comment on why it stays.

  3. Head semantics now live in one more place. The adapter re-implements ArtifactRepository._find_head for many Scopes. This follows existing precedent (runtime/statistics.py, catalog_changes/service.py), so I don't think it should block, but a cross-Scope head read on ArtifactRepository would keep "current Handoff" defined once. The existing copies already differ: statistics filters lifecycle_state == "active", while latest() does not.

I would not push this further toward a constant statement count in this PR. That means changing the shared artifact read path, and the report is already bounded by MAX_HANDOFF_REPORT_BYTES (10 MB, roughly 300 to 1100 Handoffs at 8 to 32 KB each). If Scope counts grow past that, pagination or a summary-only mode would matter more than query batching.

This branch has not been deployed

No deployments
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.

3 participants