device_memory_report: publish object names from the memory layer - #38
olehkuznetsov merged 22 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
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
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
olehkuznetsov
left a comment
There was a problem hiding this comment.
Follow-up review comments on the latest changes (9f394248d119):
…-instance GIPA unconditionally
olehkuznetsov
left a comment
olehkuznetsov
left a comment
There was a problem hiding this comment.
Code Review Summary
Thank you for the updates! The latest revisions successfully address the key lifecycle and dispatch items:
- Retiring device memory debug names now strictly follows the
DESTROYinstant trace event in bothOnFreeMemoryandOnMemoryReportEvent. downstream_extensions.resize(downstream_count)has been added invkEnumerateDeviceExtensionProperties.- The
#ifdef __ANDROID__asymmetry invkGetInstanceProcAddrhas been removed. devmemreport_EnumerateDeviceExtensionPropertieshas been cleanly inlined.
The automated multi-model consensus review ratified the following 4 remaining items:
1. [P2] Guard vkEnumerateDeviceExtensionProperties fill query on downstream_count == 0 and clamp resize()
Location: layersvt/device_memory_report/device_memory_report_handwritten_functions.h:307-322
When a downstream driver or ICD reports downstream_count == 0 on the initial count query, std::vector<VkExtensionProperties>(0).data() evaluates to nullptr. Passing nullptr to the second downstream EnumerateDeviceExtensionProperties call turns what was intended as a fill query into a redundant count query. Furthermore, downstream_extensions.resize(downstream_count) is not clamped against the pre-allocated vector size: if downstream_count grows between the two calls, resize() appends zero-initialized dummy structs (extensionName = "" and specVersion = 0).
Guarding with if (downstream_count > 0) and clamping with std::min(downstream_count, static_cast<uint32_t>(downstream_extensions.size())) matches LayerBase::EnumerateDeviceExtensionProperties:
// Manually merge device extensions when pLayerName == nullptr because the Android Vulkan
// loader does not expose device extensions from implicit layers (b/143293104).
uint32_t downstream_count = 0;
VkResult result = instance_dispatch_table(physicalDevice)->EnumerateDeviceExtensionProperties(
physicalDevice, nullptr, &downstream_count, nullptr);
if (result != VK_SUCCESS && result != VK_INCOMPLETE) {
return result;
}
constexpr uint32_t max_extensions = 4096;
downstream_count = std::min(downstream_count, max_extensions);
std::vector<VkExtensionProperties> downstream_extensions(downstream_count);
if (downstream_count > 0) {
result = instance_dispatch_table(physicalDevice)->EnumerateDeviceExtensionProperties(
physicalDevice, nullptr, &downstream_count, downstream_extensions.data());
if (result != VK_SUCCESS && result != VK_INCOMPLETE) {
return result;
}
downstream_extensions.resize(
std::min(downstream_count, static_cast<uint32_t>(downstream_extensions.size())));
}2. [P2] Adhere to Khronos Loader Policy LLP_LAYER_15 in vkEnumerateInstanceExtensionProperties and assert pPropertyCount != nullptr
Location: layersvt/device_memory_report/device_memory_report_handwritten_functions.h:243-255
Per Khronos Loader-Layer Interface Policy LLP_LAYER_15 (external/Vulkan-Loader/docs/LoaderLayerInterface.md) and LayerBase::EnumerateInstanceExtensionProperties (layersvt/common/layer_base.cpp:264-267), an unchained standalone layer's vkEnumerateInstanceExtensionProperties must return VK_ERROR_LAYER_NOT_PRESENT whenever pLayerName == nullptr or strcmp(pLayerName, LAYER_NAME) != 0. The current fallback returns VK_SUCCESS with *pPropertyCount = 0. Additionally, assert(pPropertyCount != nullptr) should be asserted before delegating to util_GetExtensionProperties, which unconditionally dereferences *pCount:
VKAPI_ATTR VkResult VKAPI_CALL vkEnumerateInstanceExtensionProperties(const char* pLayerName, uint32_t* pPropertyCount,
VkExtensionProperties* pProperties) {
if (pLayerName == nullptr || strcmp(pLayerName, LAYER_NAME) != 0) {
return VK_ERROR_LAYER_NOT_PRESENT;
}
assert(pPropertyCount != nullptr);
static const VkExtensionProperties instance_extensions[] = {
{
VK_EXT_DEBUG_UTILS_EXTENSION_NAME,
VK_EXT_DEBUG_UTILS_SPEC_VERSION,
},
};
return util_GetExtensionProperties(ARRAY_SIZE(instance_extensions), instance_extensions, pPropertyCount,
pProperties);
}3. [P3] Accept VK_INCOMPLETE in vkCreateDevice extension probe and expand abbreviations
Location: layersvt/device_memory_report/device_memory_report_handwritten_functions.h:145-169
On the second downstream EnumerateDeviceExtensionProperties call in vkCreateDevice, result != VK_SUCCESS treats VK_INCOMPLETE as a failure. In Vulkan, VK_INCOMPLETE is a valid positive status indicating that count entries were filled. Also, expand abbreviated variable names (ext_count, exts, ext, memory_report_ci) to full descriptive words:
uint32_t extension_count = 0;
VkResult result = instance_table->EnumerateDeviceExtensionProperties(
physicalDevice, nullptr, &extension_count, nullptr);
if ((result == VK_SUCCESS || result == VK_INCOMPLETE) && extension_count > 0) {
constexpr uint32_t max_extensions = 4096;
extension_count = std::min(extension_count, max_extensions);
std::vector<VkExtensionProperties> extensions(extension_count);
result = instance_table->EnumerateDeviceExtensionProperties(
physicalDevice, nullptr, &extension_count, extensions.data());
if (result == VK_SUCCESS || result == VK_INCOMPLETE) {
extensions.resize(std::min(extension_count, static_cast<uint32_t>(extensions.size())));
for (const auto& extension : extensions) {
if (strcmp(extension.extensionName, VK_EXT_DEBUG_MARKER_EXTENSION_NAME) == 0) {
has_debug_marker_extension = true;
} else if (strcmp(extension.extensionName, VK_EXT_DEBUG_UTILS_EXTENSION_NAME) == 0) {
has_debug_utils_extension = true;
}
if (has_debug_marker_extension && has_debug_utils_extension) {
break;
}
}
}
}4. [P3] Delete copy constructor and assignment on FakeInstance (Rule of 5) and expand abbreviations in tests
Location: layersvt/test/test_devicememoryreport_dispatch.cpp:197-228, 432-462
FakeDevice explicitly deletes copy construction and copy assignment (FakeDevice(const FakeDevice&) = delete; FakeDevice& operator=(const FakeDevice&) = delete;). FakeInstance manages internal pointer lifecycle (p_dev, dev) but omits copy deletion. Symmetrize the copy safety on FakeInstance, and expand test variable abbreviations (phys_dev, driver_exts, props, img_info, buf_info -> physical_device, driver_extensions, properties, image_info, buffer_info).
… fill query vkEnumerateDeviceExtensionProperties issued the downstream fill query unconditionally. When the driver reports zero extensions on the count query, std::vector<VkExtensionProperties>(0).data() is nullptr, so the "fill" call degenerates into a redundant second count query. The subsequent downstream_extensions.resize(downstream_count) was also unclamped: if the driver's count grew between the two calls, resize() appended zero-initialized dummy entries (empty extensionName, zero specVersion) that were then merged into the reported list. Skip the fill query when the count is zero, clamp the resize to the vector's allocated size, cap the allocation at 4096 extensions, and accept VK_INCOMPLETE from the count query as the valid status it is.
…sionProperties The exported vkEnumerateInstanceExtensionProperties answered queries that did not name this layer with VK_SUCCESS and an empty property list. The Khronos loader-layer interface policy LLP_LAYER_15 requires a layer to return VK_ERROR_LAYER_NOT_PRESENT when pLayerName is NULL or names a different layer, so that a caller can tell "this layer does not answer that query" apart from "this layer exposes no extensions". Also assert pPropertyCount before handing it to util_GetExtensionProperties, which dereferences it unconditionally.
…nsion probe The probe that decides whether the driver natively supports VK_EXT_device_memory_report and VK_EXT_debug_marker only accepted VK_SUCCESS from the downstream fill query. VK_INCOMPLETE is a positive status meaning the requested number of entries were written, so a driver returning it caused the layer to conclude neither extension existed and silently strip them from device creation. Accept VK_INCOMPLETE from both calls, cap the allocation at 4096 extensions, clamp the resize to the allocated size, and stop scanning once both extensions have been seen. Also spell out the abbreviated locals: ext_count, exts, ext and memory_report_ci become extension_count, extensions, extension and memory_report_create_info.
…abbreviations FakeInstance registers an instance dispatch table keyed on its own address and unregisters it in the destructor, so a copy would tear down a table it never owned. FakeDevice already deletes its copy operations; give FakeInstance the same protection. Also spell out the abbreviated test locals: img_info, buf_info, driver_exts, phys_dev and props become image_info, buffer_info, driver_extensions, physical_device and properties.
|
2 participants
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