Skip to content

Fix: recover unflushed scope stats records#1054

Merged
ChaoZheng109 merged 1 commit into
hw-native-sys:mainfrom
zmnobug:scope-stats-html-polish
Jun 17, 2026
Merged

Fix: recover unflushed scope stats records#1054
ChaoZheng109 merged 1 commit into
hw-native-sys:mainfrom
zmnobug:scope-stats-html-polish

Conversation

@zmnobug

@zmnobug zmnobug commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Recover non-empty scope_stats current buffers during host reconcile so abnormal exits can still export already-recorded records.
  • Track the recovered current buffer pointer to avoid duplicate appends if reconcile runs more than once.
  • Add C++ unit coverage for the unflushed current-buffer path and update profiling docs/comments to reflect recovery behavior.

Testing

  • conda run -n zm_pypto pre-commit run --show-diff-on-failure
  • Manual abnormal-run repro generated scope_stats JSONL/HTML from a hung paged_attention_unroll case.

Fixes #1038

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

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

Run ID: 1502d398-e24f-4e91-a52b-93706dc5df67

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
📝 Walkthrough

Walkthrough

Implements host-side recovery of un-flushed current_buf_ptr scope-stats buffers on abnormal exits (hang/op-timeout), mirroring the existing TensorDump pattern. Adds a recovered_current_buf_ tracking counter, updates reconcile_counters with conditional append and idempotency logic, aligns TensorDump comments, adds a unit test with CMake wiring, and updates all related documentation.

Changes

ScopeStats Abnormal-Exit Buffer Recovery

Layer / File(s) Summary
ScopeStatsCollector recovery logic and header
src/common/platform/include/host/scope_stats_collector.h, src/common/platform/shared/host/scope_stats_collector.cpp
Adds uint64_t recovered_current_buf_ = 0 private member, updates reconcile_counters comments to document abnormal-exit recovery semantics, and implements conditional record appending in the .cpp: resets counter in init/finalize, appends records only for a new current_buf_ptr, emits distinct "recovered" vs "already recovered" warnings, and marks clean = false plus logs an error when no host mapping exists. Drops the unused <unordered_set> include.
TensorDump comment alignment
src/common/platform/include/host/tensor_dump_collector.h, src/common/platform/shared/host/tensor_dump_collector.cpp
Rewrites reconcile_counters() doc comment from "log error and do not recover" to "recover records from non-empty DumpBufferState::current_buf_ptr", and updates the section comment header in the .cpp to match.
Unit test for reconcile recovery
tests/ut/cpp/CMakeLists.txt, tests/ut/cpp/common/test_scope_stats_collector.cpp
Adds test_scope_stats_collector CMake target (sources, includes, links, no_hardware CTest label) and the ReconcileRecoversUnflushedCurrentBuffer test: simulates a shared-memory unflushed buffer with two ScopeStatsRecord entries, asserts correct JSONL output after write_jsonl, and confirms reconcile_counters is idempotent on a second call with the same pointer.
Documentation updates
docs/profiling-framework.md, docs/dfx/scope-stats.md, docs/dfx/tensor-dump.md
Expands profiling-framework.md §1 to include TensorDump, adds per-collector links, and rewrites §8 to call out abnormal-exit current_buf_ptr recovery for TensorDump and ScopeStats. Updates the scope-stats.md §4.5 data-flow diagram and tensor-dump.md a5 lifecycle diagram and §5.5 narrative.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 When AICPU hangs and buffers stall,
No records lost — we catch them all!
recovered_current_buf_ tracks the find,
So un-flushed scopes stay left behind.
The rabbit hops the abnormal path,
And saves your stats from aftermath! 🐾

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'Fix: recover unflushed scope stats records' directly describes the primary change—recovering unflushed scope stats records during abnormal exits, which is the main focus of the changeset.
Description check ✅ Passed The PR description is clearly related to the changeset, detailing the recovery of unflushed scope_stats buffers, duplicate append prevention, unit test coverage, and documentation updates.
Linked Issues check ✅ Passed All code changes align with issue #1038 requirements: recovery of unflushed scope_stats buffers in reconcile_counters [#1038], tracking recovered buffer pointers to avoid duplicates [#1038], C++ unit test coverage [#1038], and documentation updates [#1038].
Out of Scope Changes check ✅ Passed All changes are in-scope: documentation updates reflect recovery behavior, implementation adds recovery logic to scope_stats_collector with duplicate-prevention tracking, and tests validate the unflushed buffer recovery path as specified in #1038.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request updates the profiling framework, specifically the ScopeStatsCollector and TensorDumpCollector, to recover un-flushed current buffers host-side during abnormal exits instead of performing purely passive sanity checks. It also includes corresponding documentation updates and a new unit test suite. The review feedback suggests adding an assertion in the unit test to verify that the read JSONL buffer is not empty before performing substring checks.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread tests/ut/cpp/common/test_scope_stats_collector.cpp

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/dfx/tensor-dump.md`:
- Around line 604-605: The documentation has contradictory descriptions of
reconcile_counters behavior across multiple sections. In docs/dfx/tensor-dump.md
at lines 604-605, reconcile_counters is described as actively recovering
leftover current_buf_ptr and handling dropped accounting, but the comparison
section at lines 619-623 (and any earlier lifecycle wording it references) still
describes reconcile_counters as passive or identical in behavior. Update the
comparison section at lines 619-623 and any related lifecycle descriptions to
consistently characterize reconcile_counters as an active recovery mechanism
that handles leftover buffers and dropped accounting, matching the description
at the anchor location (lines 604-605).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 80cbad4b-7329-4410-88cb-711363441608

📥 Commits

Reviewing files that changed from the base of the PR and between c5ded40 and db53da8.

📒 Files selected for processing (9)
  • docs/dfx/scope-stats.md
  • docs/dfx/tensor-dump.md
  • docs/profiling-framework.md
  • src/common/platform/include/host/scope_stats_collector.h
  • src/common/platform/include/host/tensor_dump_collector.h
  • src/common/platform/shared/host/scope_stats_collector.cpp
  • src/common/platform/shared/host/tensor_dump_collector.cpp
  • tests/ut/cpp/CMakeLists.txt
  • tests/ut/cpp/common/test_scope_stats_collector.cpp

Comment thread docs/dfx/tensor-dump.md
@zmnobug zmnobug force-pushed the scope-stats-html-polish branch 6 times, most recently from f0e3a3f to 26397c3 Compare June 16, 2026 01:30
- Recover a non-empty current scope_stats buffer during host reconcile so abnormal exits can still export already-recorded samples.

- Add a C++ unit test that simulates an unflushed current buffer and verifies recovery is not duplicated.

- Update profiling docs and stale tensor dump comments to reflect host-side recovery behavior.
@zmnobug zmnobug force-pushed the scope-stats-html-polish branch from 26397c3 to fcf0d3b Compare June 17, 2026 01:26
@ChaoZheng109 ChaoZheng109 merged commit 76c72dc into hw-native-sys:main Jun 17, 2026
16 checks passed
MirkoDeVita98 pushed a commit to MirkoDeVita98/simpler that referenced this pull request Jun 18, 2026
- Recover a non-empty current scope_stats buffer during host reconcile so abnormal exits can still export already-recorded samples.

- Add a C++ unit test that simulates an unflushed current buffer and verifies recovery is not duplicated.

- Update profiling docs and stale tensor dump comments to reflect host-side recovery behavior.

Co-authored-by: zm <zhaomin88@huawei.com>
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.

[Feature] Recover scope_stats records on the abnormal path (hang / op-timeout)

2 participants