Skip to content

mpt: make trie update stats monotonic with a snapshot API - #2502

Open
maxkozlovsky wants to merge 1 commit into
mainfrom
max/compaction-stats-monotonic
Open

mpt: make trie update stats monotonic with a snapshot API#2502
maxkozlovsky wants to merge 1 commit into
mainfrom
max/compaction-stats-monotonic

Conversation

@maxkozlovsky

Copy link
Copy Markdown
Contributor

TrieUpdateCollectedStats was reset at the start of every upsert, so a sampler outside the upsert path saw counters that vanished at the next upsert's start. Keep the totals for the lifetime of the UpdateAux instead, expose them through stats_snapshot() / last_upsert_stats(), and recover per-upsert figures by subtracting two snapshots via delta_since().

This allows exporting db stats through prometheus or similar metrics.

Also drop MONAD_MPT_COLLECT_STATS. It was hard-wired to 1 with no way to set it to 0, so its #else branches were never compiled and had rotted -- flipping it failed to build. Collection is now unconditional, which is what every build has always done.

TrieUpdateCollectedStats was reset at the start of every upsert, so a sampler
outside the upsert path saw counters that vanished at the next upsert's start.
Keep the totals for the lifetime of the UpdateAux instead, expose them through
stats_snapshot() / last_upsert_stats(), and recover per-upsert figures by
subtracting two snapshots via delta_since().

This allows exporting db stats through prometheus or similar metrics.

Also drop MONAD_MPT_COLLECT_STATS. It was hard-wired to 1 with no way to set
it to 0, so its #else branches were never compiled and had rotted -- flipping
it failed to build. Collection is now unconditional, which is what every build
has always done.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 20, 2026 19:41

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 makes trie update statistics monotonic across the lifetime of UpdateAux, enabling external samplers/metrics exporters (e.g., Prometheus) to observe counters without them being reset at the start of each upsert. It also removes the unused MONAD_MPT_COLLECT_STATS compile-time switch and makes stats collection unconditional.

Changes:

  • Replace per-upsert resettable stats with lifetime totals (stats_) plus snapshot/delta tracking (prev_upsert_stats_, last_upsert_stats_) and expose them via stats_snapshot() / last_upsert_stats().
  • Compute per-upsert figures as delta_since() between consecutive snapshots and plumb those deltas into logging and compaction speed-control logic.
  • Add a unit test asserting that last_upsert_stats() matches snapshot deltas across consecutive upserts (including when compaction=false).

Verdict: CORRECT

Generated with Claude Code

Reviewed changes

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

Show a summary per file
File Description
category/mpt/update_aux.cpp Computes/stores per-upsert deltas from monotonic lifetime totals; updates logging and compaction logic to consume the delta.
category/mpt/trie.hpp Adds snapshot APIs and moves stats members to private lifetime-tracking fields.
category/mpt/trie.cpp Removes the per-upsert reset_stats() call.
category/mpt/test/compaction_test.cpp Adds coverage ensuring last_upsert_stats() isolates the latest upsert and matches snapshot differences.
category/mpt/detail/collected_stats.hpp Removes MONAD_MPT_COLLECT_STATS, makes the struct always full-sized, and adds delta_since() API.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions github-actions Bot 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.

Reviewed the change. The refactor keeps TrieUpdateCollectedStats as a lifetime accumulator inside UpdateAux, computes each upsert's contribution as a delta between snapshots at the end of do_update, and threads the delta explicitly into print_update_stats. That preserves the semantics that mattered:

  • advance_compact_offsets needs "bytes compacted to slow during the previous upsert" to set gc_efficiency. Under the old code, it read stats.compacted_bytes_in_slow before upsert()'s reset_stats() fired, so it saw the previous upsert's total. last_upsert_stats_.compacted_bytes_in_slow is now written unconditionally at end of do_update and is the exact same quantity, since every collect_* call happens inside upsert() and cannot fire between do_update invocations.
  • The unconditional assignment before the if (compaction) block correctly ensures a compaction=false upsert's counters can't leak into the next compacted upsert's delta — and the new test pins that behavior.
  • delta_since() covers all 16 named fields (including both [2] arrays via the loop), and the sizeof == 160 and sizeof(UpdateAux) == 120 + 3 * sizeof(TrieUpdateCollectedStats) static_asserts will fire if a field is added, forcing the author to look at delta_since() too.
  • Grep confirms no external consumers of the removed aux.stats, reset_stats, or MONAD_MPT_COLLECT_STATS; the #include <new> drop matches the placement-new removal.
  • The added test uses the existing FillDBWithChunksGTest fixture and validates the monotonicity + delta-isolation invariants that a reintroduced reset would break.

Style, naming, and PR hygiene (single commit, #pragma once, trailing-underscore private members, east-const on parameter definitions) all check out.

Verdict: CORRECT

🤖 Generated with Claude Code

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