device_memory_report: publish object names from the memory layer - #38
jimblacklercorp wants to merge 18 commits into
Conversation
The memory view labels allocations with the debug names an application gives its objects. Those names were only available from VK_LAYER_GOOGLE_DebugMarker, so Sherlock had to load that layer whenever the memory report was enabled, paying for full API event tracing to get a handful of names. Intercept vkSetDebugUtilsObjectNameEXT and vkDebugMarkerSetObjectNameEXT here instead and publish the names as VulkanObjectName instant events under the VulkanDeviceMemoryReport category, alongside the events they annotate. Names are replayed from DumpCurrentCountersAndAllocations so sessions that attach after the application named its objects still see them, and repeated naming of an unchanged name is dropped because applications re-apply names routinely. Only buffers, images and device memory are tracked, since the memory view cannot attribute memory to anything else. Applications that need every object named, to label GPU render stages for example, are still served by the debug marker layer. The layer does not advertise VK_EXT_debug_marker the way the debug marker layer does, so it stays passive: vkGetDeviceProcAddr only hands out these intercepts when the layer below implements them, and an extension never appears available because this layer is loaded. BUG=b/559839199
olehkuznetsov
left a comment
There was a problem hiding this comment.
Overview & Assessment
Thank you for adding Vulkan object debug name tracking and periodic snapshot replay to VK_LAYER_GOOGLE_DeviceMemoryReport! Keying names by (VkObjectType, uint64_t object_handle) and republishing them during DumpCurrentCountersAndAllocations() is well-structured and addresses b/559839199 cleanly.
The core implementation is solid. The inline comments focus on:
- Resolving the contradiction between the handwritten interceptor fallback branches (
: VK_SUCCESS) and the dispatch table gates (down_func != nullptr). - Enforcing non-null pointer contracts via assertions instead of silent success returns (
pNameInfo == nullptr). - Making
object_typemandatory onOnDestroyObjectat compile time to prevent silent name leaks. - Removing redundant cleanup in
RemoveAllocationTracking. - Ensuring test hermeticity across
--gtest_repeatand adding dispatch-level interceptor coverage.
olehkuznetsov
left a comment
There was a problem hiding this comment.
Overview & Assessment
Thank you for continuing to refine the DeviceMemoryReport Vulkan object debug naming support! Following up on the latest commits in this PR, here is the consolidated consensus code review across the implementation.
The core naming mechanism and Perfetto event emission are well-structured, but there are a few important correctness and contract issues that need resolution:
- Commit Description vs. Implementation (F1 — P1): The commit message claims the layer "stays passive: vkGetDeviceProcAddr only hands out these intercepts when the layer below implements them, and an extension never appears available because this layer is loaded", whereas the implementation operates as an active standalone producer (advertising extensions in the manifest, injecting them in
vkEnumerate*ExtensionProperties, and returning intercepts unconditionally). The author should explicitly decide between Option A (updating the description/headers to document the standalone producer model) and Option B (reverting to passive interception). - Android Extension Enumeration (F2 — P2):
vkEnumerateDeviceExtensionPropertieshas an asymmetry between the count query (+= 2) and the deduplicating fill query, returnsVK_SUCCESSon zero count instead ofVK_INCOMPLETE, uses defensive returns, and is restricted behind#ifdef __ANDROID__. vkCreateDebugUtilsMessengerEXTStub (F3 — P2): The stub returnsVK_SUCCESSwithout modifying*pMessenger, leaving caller handles uninitialized if downstream lacks the entry point.- Minor Polish (P3s): Scope or explain
ENABLE_EXPORTSon test binaries (F4), remove unreachable GDPA core table entry (F5), deduplicateDeviceMemoryReportTestPeer(F6), and emit an empty-name event when erasing on destroy/free to handle handle recycling (F7).
…m core device dispatch
olehkuznetsov
left a comment
There was a problem hiding this comment.
Follow-up review comments on the latest changes (9f394248d119):
…-instance GIPA unconditionally
The memory view labels allocations with the debug names an application gives its objects. Those names were only available from VK_LAYER_GOOGLE_DebugMarker, so Sherlock had to load that layer whenever the memory report was enabled, paying for full API event tracing to get a handful of names.
Intercept vkSetDebugUtilsObjectNameEXT and vkDebugMarkerSetObjectNameEXT here instead and publish the names as VulkanObjectName instant events under the VulkanDeviceMemoryReport category, alongside the events they annotate. Names are replayed from DumpCurrentCountersAndAllocations so sessions that attach after the application named its objects still see them, and repeated naming of an unchanged name is dropped because applications re-apply names routinely.
Only buffers, images and device memory are tracked, since the memory view cannot attribute memory to anything else. Applications that need every object named, to label GPU render stages for example, are still served by the debug marker layer.
To allow the layer to operate standalone without VK_LAYER_GOOGLE_DebugMarker loaded, it advertises VK_EXT_debug_utils and VK_EXT_debug_marker in its manifest and extension enumeration entrypoints, strips VK_EXT_debug_marker from vkCreateDevice when the downstream driver does not natively support it, and provides no-op passthroughs for companion commands in both extensions when no lower layer or driver implements them.
BUG=b/559839199