Skip to content

device_memory_report: emit instant events for snapshot report - #36

Merged
olehkuznetsov merged 21 commits into
android-graphics:mainfrom
jimblacklercorp:feature-device-memory-report-snapshot-events
Sep 17, 2026
Merged

olehkuznetsov merged 21 commits into
android-graphics:mainfrom
jimblacklercorp:feature-device-memory-report-snapshot-events

Conversation

@jimblacklercorp

Copy link
Copy Markdown

Emit events on trace session start and live events.

Allows tracking of memory usage over time.

BUG=b/559839199

Emit events on trace session start and live events.

Allows tracking of memory usage over time.

BUG=b/559839199

@olehkuznetsov olehkuznetsov 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 Summary for PR #36 (vmpxxnwsswos)

Thank you for adding Perfetto snapshot dumping and memory tracking events! Enabling state reconstruction for trace sessions started mid-execution is a valuable improvement.

Key Observations & Findings:

  1. [P1] Missing RemoveSessionObserver in Destructor (device_memory_report_perfetto.cpp):
    g_session_observer must unregister via perfetto::TrackEvent::RemoveSessionObserver(this) in its destructor to prevent a dangling pointer in Perfetto's static registry after layer unloading (dlclose), which leads to SIGSEGV on subsequent trace sessions.
  2. [P1] Pre-Existing Tracing Sessions Miss Snapshot (device_memory_report_perfetto.cpp):
    AddSessionObserver only fires for future session starts. If tracing is already running when the Vulkan instance is created (standard ADB profiling flow), checking TRACE_EVENT_CATEGORY_ENABLED upon initialization ensures the snapshot is captured.
  3. [P1] Phantom BIND Instant Event for Unbound Memory (device_memory_report.cpp):
    Emitting operation: "BIND" with offset: 0 for applied_unbound_bytes invents a synthetic binding that collides with real resources at offset 0 and diverges from live tracing (which tracks unbound memory via TRACE_COUNTER). This block should be removed.
  4. [P1] Dead Write-Only heap_index & Semantic Conflation (device_memory_report.h / .cpp):
    allocation.heap_index is never read, and assigning memory_type_index into it conflates memory type index with heap index. Also remove the unused local variable heap_idx.
  5. [P1/P2] Unnecessary std::string Allocations on Hot Paths (device_memory_report.cpp):
    GetCluster() returns const char*. Using const char* directly eliminates heap allocation churn on resource binding/unbinding paths even when tracing is disabled.
  6. [P1] Vacuous EXPECT_TRUE(true) in Snapshot Test (test_devicememoryreport.cpp):
    Convert the smoke test into a meaningful regression test using DeviceMemoryReportTestPeer to assert tracked allocation state, suballocations, unbound bytes, and teardown cleanup.
  7. [P2] Naming & Commit Scope:
    Expand abbreviated identifiers (is_img, sub_size, src_str, op_str, cb_data) to full descriptive names per project style.

Please see the detailed inline review comments for line-by-line suggestions.

Comment thread layersvt/device_memory_report/device_memory_report_perfetto.cpp Outdated
Comment thread layersvt/device_memory_report/device_memory_report_perfetto.cpp Outdated
Comment thread layersvt/device_memory_report/device_memory_report.h
Comment thread layersvt/device_memory_report/device_memory_report.h Outdated
Comment thread layersvt/device_memory_report/device_memory_report.cpp Outdated
Comment thread layersvt/device_memory_report/device_memory_report.cpp Outdated
Comment thread layersvt/device_memory_report/device_memory_report.cpp Outdated
Comment thread layersvt/device_memory_report/device_memory_report.cpp Outdated
Comment thread layersvt/device_memory_report/device_memory_report.cpp Outdated
Comment thread layersvt/test/test_devicememoryreport.cpp Outdated

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

Review Summary: Instant Events & Snapshot Dump for VK_LAYER_GOOGLE_device_memory_report

Thanks for adding live and session-start snapshot instant event tracing (VulkanMemoryAllocation) to DeviceMemoryReport!

Overall, the architecture for capturing session-start state via TrackEventSessionObserver is solid. Before merging, there are a few lifecycle, schema-consistency, and commit-hygiene items to address:

  1. C++ Static Destruction Order (device_memory_report_perfetto.cpp): g_session_observer is initialized at library load time (.init_array), whereas DeviceMemoryReport::Get() is initialized lazily on first call. By C++ LIFO static destruction rules ([basic.start.term]), ~DeviceMemoryReport() destroys counter_mutex_ and internal maps before ~DeviceMemoryReportSessionObserver() unregisters from Perfetto.
  2. Synthetic Unbound BIND Event in Snapshot (device_memory_report.cpp:423-432): Emitting a synthetic "operation", "BIND" event at offset = 0 for unbound_memory diverges from live tracing (which tracks unbound headroom via TRACE_COUNTER), spatially overlaps resources bound at offset 0, and is never retired or shrunk when subsequent live resources bind to the slab.
  3. Suballocation & Fallback DESTROY Completeness (device_memory_report.cpp):
    • When a VkDeviceMemory slab is freed while resources are still bound to it (which is legal in Vulkan per Spec Section 11.2.13 and occurs in MemoryReportSnapshotDump), RemoveAllocationTracking clears resource_to_memory_map_ without emitting suballocation DESTROY events, causing subsequent OnDestroyObject calls to silently drop DESTROY events.
    • In OnFreeMemory, calling RemoveAllocationTracking(handle) before reading total_size hardcodes "size", 0 and emits phantom DESTROY events for VK_NULL_HANDLE / untracked handles.
    • In OnMemoryReportEvent, driver DESTROY (FREE_EXT) events always fall back to "unbound_memory" because vkDestroyImage/vkDestroyBuffer calls OnDestroyObject (erasing resources_[object_handle]) before downstream fpDestroyImage/fpDestroyBuffer fires FREE_EXT.
  4. Commit History Hygiene: Commits 2–8 (vxwltluqnlvu..omqunmrtmqvt) are incremental fixups repairing defects introduced in Commit 1 (vmpxxnwsswos, e.g., dead heap_index member, missing observer teardown, EXPECT_TRUE(true) test). Please squash this stack into 1 atomic commit (or 2 commits: implementation + test) before merging so intermediate broken states are not preserved in git bisect history.

Comment thread layersvt/device_memory_report/device_memory_report_perfetto.cpp
Comment thread layersvt/device_memory_report/device_memory_report.h
Comment thread layersvt/device_memory_report/device_memory_report.cpp Outdated
Comment thread layersvt/device_memory_report/device_memory_report.cpp Outdated
Comment thread layersvt/device_memory_report/device_memory_report.cpp
Comment thread layersvt/device_memory_report/device_memory_report.cpp Outdated
Comment thread layersvt/device_memory_report/device_memory_report.cpp
Comment thread layersvt/test/test_devicememoryreport.cpp
Comment thread layersvt/device_memory_report/device_memory_report.cpp Outdated
Return early when handling FREE_EXT or UNIMPORT_EXT for allocations
that are not tracked in memory_allocations_, preventing phantom DESTROY
trace events from being emitted.

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

This is a strong series — the static destruction ordering fix in 62c14e418 ("fix static destruction order of session observer"), dropping the synthetic unbound BIND in 91d221c42 ("remove synthetic unbound BIND event and string copies in snapshot dump"), and the driver-keying alignment in 888957aef ("align key with non-driver check and use cached cluster_name in OnMemoryReportEvent") all hold up under scrutiny. The locked std::optional test peer is the right shape, and I traced the new test's counter arithmetic end to end — it's net-zero, so it's safe under --gtest_shuffle.

One real issue: OnMemoryReportEvent reports a spec-undefined size on DESTROY. You already fixed the identical thing in OnFreeMemory two commits earlier, so I think this is just an oversight. One-line fix.

Comment thread layersvt/device_memory_report/device_memory_report.cpp Outdated
Comment thread layersvt/device_memory_report/device_memory_report.h
@olehkuznetsov
olehkuznetsov merged commit fbd75a1 into android-graphics:main Sep 17, 2026
15 checks passed
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