Skip to content

Bucket query asserts and perf fix#5323

Open
SirTyson wants to merge 2 commits into
stellar:masterfrom
SirTyson:bucket-query-asserts
Open

Bucket query asserts and perf fix#5323
SirTyson wants to merge 2 commits into
stellar:masterfrom
SirTyson:bucket-query-asserts

Conversation

@SirTyson

Copy link
Copy Markdown
Contributor

Description

The first commit resolves #5067.

The 2nd commit addresses a perf issue we saw with the BucketList refactor. We build snapshot objects in parallel contents. Currently, the constructor gets a fresh set of metric references for each snapshot, acquiring a lock to do so. This changes it such that we use a single shared metrics object across all snapshots. Metrics are thread safe, so there should be no safety issue with this, and it makes the snapshot constructor much cheaper.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@SirTyson SirTyson requested review from Copilot and dmkozh June 11, 2026 22:54
@SirTyson SirTyson changed the title Bucket query asserts Bucket query asserts and perf fix Jun 11, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4bdff954c0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/bucket/BucketListSnapshot.cpp Outdated

Copilot AI 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.

Pull request overview

This PR improves BucketList snapshot safety and performance by (1) adding a test-build thread-ownership invariant to detect illegal cross-thread reuse of a single snapshot instance (per #5067), and (2) reducing snapshot/view construction overhead by sharing pre-resolved metric handles across views over the same immutable ledger state.

Changes:

  • Add a threadInvariant() assertion path (BUILD_TESTS) to SearchableBucketListSnapshot query entry points to catch cross-thread snapshot reuse.
  • Introduce BucketSnapshotMetrics and store shared metric references in ImmutableLedgerData so per-view snapshot construction avoids repeatedly taking the global metrics registry lock.
  • Thread metrics registry through ImmutableLedgerData construction from LedgerManagerImpl.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/ledger/LedgerManagerImpl.cpp Passes MetricsRegistry into ImmutableLedgerData construction sites.
src/ledger/ImmutableLedgerView.h Extends ImmutableLedgerData to hold shared BucketSnapshotMetrics and updates ctor signature.
src/ledger/ImmutableLedgerView.cpp Constructs shared snapshot metrics once per immutable state and wires them into ImmutableLedgerView snapshots.
src/bucket/BucketListSnapshot.h Adds thread-invariant state (test builds) and introduces BucketSnapshotMetrics shared across snapshots.
src/bucket/BucketListSnapshot.cpp Implements BucketSnapshotMetrics, shares metrics in snapshot ctor, and adds thread-invariant checks on query paths.

Comment thread src/bucket/BucketListSnapshot.h
Comment thread src/bucket/BucketListSnapshot.h Outdated
Comment thread src/bucket/BucketListSnapshot.cpp Outdated
@SirTyson SirTyson force-pushed the bucket-query-asserts branch from 4bdff95 to a8d0c5f Compare June 11, 2026 23:03
@SirTyson SirTyson enabled auto-merge June 11, 2026 23:46
@SirTyson SirTyson added this pull request to the merge queue Jun 11, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Jun 12, 2026
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.

Consider adding thread invariance to Bucket snapshots

3 participants