Skip to content

layersvt: Remove DebugMarker object names on null or empty name #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
olehkuznetsov wants to merge 1 commit into
base: pr-debugmarker-device-key
Choose a base branch
from
+63 −21
Open

layersvt: Remove DebugMarker object names on null or empty name #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions layersvt/debug_marker/debug_marker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ namespace {

// The packet shares the track event sequence, whose defaults declare an incremental clock, so the
// absolute timestamp must carry an explicit clock id to not be read as a delta.
// An empty object_name, emitted by RemoveDebugObjectName, means the object is unnamed from this
// packet's timestamp on.
void EmitDebugObjectNamePacket(uint64_t device, int32_t type, uint64_t handle, const std::string& name) {
perfetto::TrackEvent::Trace([device, type, handle, &name](perfetto::TrackEvent::TraceContext ctx) {
auto packet = ctx.NewTracePacket();
Expand Down Expand Up @@ -94,13 +96,25 @@ void DebugMarker::PreDestroyDevice(VkDevice device, const VkAllocationCallbacks*
}

void DebugMarker::SetDebugObjectName(uint64_t device, int32_t type, uint64_t handle, const char* name) {
if (name == nullptr || name[0] == '\0') {
RemoveDebugObjectName(device, type, handle);
return;
}

std::lock_guard<std::mutex> lock(mutex_);

std::string name_str = name ? name : "NULL";
std::string name_str = name;
debug_object_names_[std::make_tuple(device, type, handle)] = DebugObjectName(device, type, handle, name_str);
EmitDebugObjectNamePacket(device, type, handle, name_str);
}

void DebugMarker::RemoveDebugObjectName(uint64_t device, int32_t type, uint64_t handle) {
std::lock_guard<std::mutex> lock(mutex_);
if (debug_object_names_.erase(std::make_tuple(device, type, handle)) == 0) return;
// An empty name tells trace consumers the object is unnamed from here on.
EmitDebugObjectNamePacket(device, type, handle, std::string());
}

void DebugMarker::EmitAllDebugMarkers() {
std::lock_guard<std::mutex> lock(mutex_);
for (const auto& entry : debug_object_names_) {
Expand All @@ -109,11 +123,11 @@ void DebugMarker::EmitAllDebugMarkers() {
}
}

bool DebugMarker::HasDebugObjectName(uint64_t device, int32_t type, uint64_t handle, const std::string& name) {
std::optional<std::string> DebugMarker::GetDebugObjectName(uint64_t device, int32_t type, uint64_t handle) const {
std::lock_guard<std::mutex> lock(mutex_);
auto it = debug_object_names_.find(std::make_tuple(device, type, handle));
if (it == debug_object_names_.end()) return false;
return it->second.name == name;
if (it == debug_object_names_.end()) return std::nullopt;
return it->second.name;
}

VkResult DebugMarker::CreateEmulatedMessenger(VkInstance instance, const VkDebugUtilsMessengerCreateInfoEXT* pCreateInfo,
Expand Down
18 changes: 13 additions & 5 deletions layersvt/debug_marker/debug_marker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <mutex>

#include <map>
#include <optional>
#include <string>
#include <tuple>
#include <vector>
Expand Down Expand Up @@ -67,20 +68,27 @@ class DebugMarker : public layersvt::LayerBase {
* @param device The handle of the Vulkan device that owns the object.
* @param type The type of the Vulkan object (represented as int32_t).
* @param handle The handle of the Vulkan object.
* @param name The name to associate with the object.
* @param name The name to associate with the object. A null or empty name removes the object's name,
* as VK_EXT_debug_utils defines; the same rule is applied to VK_EXT_debug_marker.
*/
void SetDebugObjectName(uint64_t device, int32_t type, uint64_t handle, const char* name);

/**
* Removes the name associated with a Vulkan object. If the object was named, emits an empty
* name to the trace so consumers treat it as unnamed from then on.
*/
void RemoveDebugObjectName(uint64_t device, int32_t type, uint64_t handle);

/**
* @brief Emits all stored debug markers to the tracing system.
*/
void EmitAllDebugMarkers();

/**
* @brief Checks if a debug name is stored for a given object.
* @note This function is for testing only.
* Returns the name stored for a Vulkan object, or std::nullopt if the object has no name.
* For testing only.
*/
bool HasDebugObjectName(uint64_t device, int32_t type, uint64_t handle, const std::string& name);
std::optional<std::string> GetDebugObjectName(uint64_t device, int32_t type, uint64_t handle) const;

/**
* @brief Emulated VK_EXT_debug_utils messengers, used for instances whose downstream lacks the
Expand Down Expand Up @@ -127,7 +135,7 @@ class DebugMarker : public layersvt::LayerBase {
: vk_device(dev), object_type(type), handle(h), name(n) {}
};

std::mutex mutex_;
mutable std::mutex mutex_;
/**
* @brief Maps (vk_device, object_type, object_handle) to its debug name information.
* Non-dispatchable handles are not guaranteed to be unique across object types or devices.
Expand Down
44 changes: 32 additions & 12 deletions layersvt/test/test_debugmarker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,15 @@ TEST_F(DebugMarkerTests, CombinedTest) {
// 1. Set instance name
DebugMarker::Get().SetDebugObjectName(0, VK_OBJECT_TYPE_INSTANCE, (uint64_t)instance, "MyInstance");

EXPECT_TRUE(DebugMarker::Get().HasDebugObjectName(0, VK_OBJECT_TYPE_INSTANCE, (uint64_t)instance, "MyInstance"));
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(0, VK_OBJECT_TYPE_INSTANCE, (uint64_t)instance), "MyInstance");

// 2. Override new name
DebugMarker::Get().SetDebugObjectName(0, VK_OBJECT_TYPE_INSTANCE, (uint64_t)instance, "MyInstanceRenamed");
EXPECT_TRUE(DebugMarker::Get().HasDebugObjectName(0, VK_OBJECT_TYPE_INSTANCE, (uint64_t)instance, "MyInstanceRenamed"));
EXPECT_FALSE(DebugMarker::Get().HasDebugObjectName(0, VK_OBJECT_TYPE_INSTANCE, (uint64_t)instance, "MyInstance"));
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(0, VK_OBJECT_TYPE_INSTANCE, (uint64_t)instance), "MyInstanceRenamed");

// 3. Reset
layer_test::ResetLayer<DebugMarker>();
EXPECT_FALSE(DebugMarker::Get().HasDebugObjectName(0, VK_OBJECT_TYPE_INSTANCE, (uint64_t)instance, "MyInstanceRenamed"));
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(0, VK_OBJECT_TYPE_INSTANCE, (uint64_t)instance), std::nullopt);
}

TEST_F(DebugMarkerTests, ManifestTest) {
Expand Down Expand Up @@ -159,18 +158,39 @@ TEST_F(DebugMarkerTests, PreDestroyDeviceCleanupTest) {
DebugMarker::Get().SetDebugObjectName((uint64_t)dev1, VK_OBJECT_TYPE_BUFFER, 0x1111, "Buffer1");
DebugMarker::Get().SetDebugObjectName((uint64_t)dev2, VK_OBJECT_TYPE_BUFFER, 0x2222, "Buffer2");

EXPECT_TRUE(DebugMarker::Get().HasDebugObjectName((uint64_t)dev1, VK_OBJECT_TYPE_BUFFER, 0x1111, "Buffer1"));
EXPECT_TRUE(DebugMarker::Get().HasDebugObjectName((uint64_t)dev2, VK_OBJECT_TYPE_BUFFER, 0x2222, "Buffer2"));
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName((uint64_t)dev1, VK_OBJECT_TYPE_BUFFER, 0x1111), "Buffer1");
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName((uint64_t)dev2, VK_OBJECT_TYPE_BUFFER, 0x2222), "Buffer2");

// Destroy dev1 - should remove Buffer1 but keep Buffer2
layersvt::LayerBaseTestPeer::DestroyDevice(dev1, nullptr);

EXPECT_FALSE(DebugMarker::Get().HasDebugObjectName((uint64_t)dev1, VK_OBJECT_TYPE_BUFFER, 0x1111, "Buffer1"));
EXPECT_TRUE(DebugMarker::Get().HasDebugObjectName((uint64_t)dev2, VK_OBJECT_TYPE_BUFFER, 0x2222, "Buffer2"));
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName((uint64_t)dev1, VK_OBJECT_TYPE_BUFFER, 0x1111), std::nullopt);
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName((uint64_t)dev2, VK_OBJECT_TYPE_BUFFER, 0x2222), "Buffer2");

// Destroy dev2 - should remove Buffer2
layersvt::LayerBaseTestPeer::DestroyDevice(dev2, nullptr);
EXPECT_FALSE(DebugMarker::Get().HasDebugObjectName((uint64_t)dev2, VK_OBJECT_TYPE_BUFFER, 0x2222, "Buffer2"));
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName((uint64_t)dev2, VK_OBJECT_TYPE_BUFFER, 0x2222), std::nullopt);
}

TEST_F(DebugMarkerTests, NullOrEmptyNameRemovesNameTest) {
TEST_DESCRIPTION("Verify a null or empty object name removes the stored name instead of storing it");

const uint64_t device = 0x1000;
DebugMarker::Get().SetDebugObjectName(device, VK_OBJECT_TYPE_BUFFER, 0x1, "Buffer");
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(device, VK_OBJECT_TYPE_BUFFER, 0x1), "Buffer");
DebugMarker::Get().SetDebugObjectName(device, VK_OBJECT_TYPE_BUFFER, 0x1, nullptr);
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(device, VK_OBJECT_TYPE_BUFFER, 0x1), std::nullopt);

DebugMarker::Get().SetDebugObjectName(device, VK_OBJECT_TYPE_IMAGE, 0x2, "Image");
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(device, VK_OBJECT_TYPE_IMAGE, 0x2), "Image");
DebugMarker::Get().SetDebugObjectName(device, VK_OBJECT_TYPE_IMAGE, 0x2, "");
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(device, VK_OBJECT_TYPE_IMAGE, 0x2), std::nullopt);

// Removing the name of an object that was never named leaves other names untouched.
DebugMarker::Get().SetDebugObjectName(device, VK_OBJECT_TYPE_BUFFER, 0x3, "Other");
DebugMarker::Get().RemoveDebugObjectName(device, VK_OBJECT_TYPE_BUFFER, 0x4);
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(device, VK_OBJECT_TYPE_BUFFER, 0x3), "Other");
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(device, VK_OBJECT_TYPE_BUFFER, 0x4), std::nullopt);
}

TEST_F(DebugMarkerTests, SameHandleOnDifferentDevicesTest) {
Expand All @@ -181,8 +201,8 @@ TEST_F(DebugMarkerTests, SameHandleOnDifferentDevicesTest) {
DebugMarker::Get().SetDebugObjectName(device1, VK_OBJECT_TYPE_BUFFER, 0x1, "Device1Buffer");
DebugMarker::Get().SetDebugObjectName(device2, VK_OBJECT_TYPE_BUFFER, 0x1, "Device2Buffer");

EXPECT_TRUE(DebugMarker::Get().HasDebugObjectName(device1, VK_OBJECT_TYPE_BUFFER, 0x1, "Device1Buffer"));
EXPECT_TRUE(DebugMarker::Get().HasDebugObjectName(device2, VK_OBJECT_TYPE_BUFFER, 0x1, "Device2Buffer"));
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(device1, VK_OBJECT_TYPE_BUFFER, 0x1), "Device1Buffer");
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName(device2, VK_OBJECT_TYPE_BUFFER, 0x1), "Device2Buffer");
}

TEST_F(DebugMarkerTests, TemplateMethodDispatchTest) {
Expand Down Expand Up @@ -264,7 +284,7 @@ TEST_F(DebugMarkerTests, MissingDownstreamExtensionTest) {
name_info.objectHandle = 0x4444;
name_info.pObjectName = "Buffer";
EXPECT_EQ(set_object_name(mock_device, &name_info), VK_SUCCESS);
EXPECT_TRUE(DebugMarker::Get().HasDebugObjectName((uint64_t)mock_device, VK_OBJECT_TYPE_BUFFER, 0x4444, "Buffer"));
EXPECT_EQ(DebugMarker::Get().GetDebugObjectName((uint64_t)mock_device, VK_OBJECT_TYPE_BUFFER, 0x4444), "Buffer");
}

namespace {
Expand Down