Support multiple simultaneous API capture managers #1536

jzulauf-lunarg · 2024-05-03T22:54:32Z

Split common and api specific options from isA to refsA relationship.

For single API sessions should be exactly equivalent.

This is a first piece of multiple API support needed for pending feature development.

Issues to be resolved:
Settings Arbitration: different API invocations of the layer may have different settings. Determining where are complete set of options are (without having to repeat them in every JSON) is one challenge. Resolving which setting "wins" (first, last, some priority heuristic). Settings side effects (like whether an StateTracker is create) need to be address if later invocations can change settings.

A number of interfaces became "public" to support this. The other choice is friendship between the various CaptureManagers, thoughts?

Also, concerns about performance or maintainability.

ci-tester-lunarg · 2024-05-03T22:54:36Z

CI gfxreconstruct build queued with queue ID 179237.

CLAassistant · 2024-05-03T22:54:38Z

All committers have signed the CLA.

ci-tester-lunarg · 2024-05-03T22:55:01Z

CI gfxreconstruct build # 4103 running.

ci-tester-lunarg · 2024-05-03T23:18:15Z

CI gfxreconstruct build # 4103 failed.

framework/encode/capture_manager.h

jzulauf-lunarg

Added some comment for reviewers.

jzulauf-lunarg · 2024-05-06T14:57:19Z

framework/encode/capture_manager.cpp

@@ -51,12 +51,11 @@ std::mutex                                     CaptureManager::ThreadData::count
 format::ThreadId                               CaptureManager::ThreadData::thread_count_ = 0;
 std::unordered_map<uint64_t, format::ThreadId> CaptureManager::ThreadData::id_map_;

-uint32_t                                                 CaptureManager::instance_count_ = 0;
+CaptureManager*                                          CaptureManager::singleton_;


Note the change of terminology from "instance" to "singleton" throughout the PR. The singleton pattern implemented maps many API instances to the same CaptureManager singletons, thus calling the "singletons" "instances" seems quite confusing, given that a CreateInstance call may correlate to 0 or 1 CaptureManager creations, and DestroyInstance may correlate to 0 or 1 CaptureManager destructions.

jzulauf-lunarg · 2024-05-06T14:59:21Z

framework/encode/capture_manager.cpp

@@ -89,8 +88,8 @@ format::ThreadId CaptureManager::ThreadData::GetThreadId()
    return id;
 }

-CaptureManager::CaptureManager(format::ApiFamilyId api_family) :
-    api_family_(api_family), force_file_flush_(false), timestamp_filename_(true),
+CaptureManager::CaptureManager() :


The CaptureManager changes from being a base class to a referenced common class. All API specific capture managers access the common functionality through a referenced instance of the common class.

Please add this comment to the source itself.

jzulauf-lunarg · 2024-05-06T15:03:00Z

framework/encode/capture_manager.cpp

        // Initialize logging to report only errors (to stderr).
        util::Log::Settings stderr_only_log_settings;
        stderr_only_log_settings.min_severity            = util::Log::kErrorSeverity;
        stderr_only_log_settings.output_errors_to_stderr = true;
        util::Log::Init(stderr_only_log_settings);

+        // WIP WIP -- FIRST WINS for settings


Since the current implementation doesn't support multiple disparate API's being captured, the "first wins" settings limitation doesn't impact current functionality, but needs to be resolved for actual multi API support (which is need for specific future use cases).

WIP comment is to attract reviewers attention, to be changed to NOTE or TODO prior to commit.

jzulauf-lunarg · 2024-05-06T15:06:00Z

framework/encode/capture_manager.cpp

+        GFXRECON_ASSERT(inserted.second);
+        manager_it = inserted.first;
+
+        // WIP NOTE moved here from CaptureTracker::Initialize... DRY'r than putting it into the API specific


Folks with a better understand of why StateTracker objects are lazily created please comment. Would it be reasonable to always create an empty state tracker, even if unused, s.t. eventual interactions between multiple capture managers settings don't lead to some inconsistent state?

jzulauf-lunarg · 2024-05-06T15:44:36Z

framework/encode/capture_manager.cpp

    GFXRECON_LOG_DEBUG("CaptureManager::CreateInstance(): Current instance count is %u", instance_count_);

    return success;
 }

-void CaptureManager::DestroyInstance(std::function<const CaptureManager*()> GetInstanceFunc)
+void CaptureManager::DestroyInstance(ApiCaptureManagerBase* api_capture_manager)


The destruction function is now captured at CreateInstance time. The "delete on last" logic is implemented here in the common class.

jzulauf-lunarg · 2024-05-06T15:52:58Z

framework/encode/capture_manager.cpp

@@ -1093,7 +1168,7 @@ void CaptureManager::WriteFillMemoryCmd(format::HandleId memory_id, uint64_t off

 void CaptureManager::WriteCreateHeapAllocationCmd(uint64_t allocation_id, uint64_t allocation_size)
 {
-    if ((GetCaptureMode() & kModeWrite) == kModeWrite)
+    if (IsCaptureModeWrite())


Class segregation moved the definition such that it would need qualification, and since the expression needed rewrite, seemed DRY'r to encapsulate.

jzulauf-lunarg · 2024-05-06T15:54:56Z

framework/encode/capture_manager.cpp

 void CaptureManager::WriteCaptureOptions(std::string& operation_annotation)
 {
-    CaptureSettings::TraceSettings default_settings = GetDefaultTraceSettings();
+    CaptureSettings::TraceSettings default_settings = default_settings_.GetTraceSettings();


The common capture manager sets this up, getting information from the capture manager associated with the first CreateInstance... seemed simplest to cache that data away, rather than track who's on first.

framework/encode/capture_manager.h

jzulauf-lunarg · 2024-05-06T16:00:00Z

framework/encode/capture_manager.h

@@ -374,6 +388,178 @@ class CaptureManager
    } rv_annotation_info_;
 };

+class ApiCaptureManagerBase


My biggest concerns here are maintainability and performance (with the additional indirection). Not sure of a different approach that would require a lookup of CaptureManager::singleton_ everywhere one of these methods is invoked.

Another possible idea for breaking apart these classes: Have the common capture manager and each API capture manager be singletons and require the calling code (e.g., in the API-specific capture manager classes, or in the generated encoder files) to know which class to call into. Then the API manager classes don't need to pass-through all the common class methods and might help promote a division of responsibilities between the common and API-specific capture managers.

The API capture managers could still store a pointer to the common capture manager for cases when they need to call into it.

The goal of the ApiCaptureManagerBase passthrough calls was to minimize the code change impact of call into the common CaptureManager. Without this, every common call would need to be changed to be prefixed by common_manager_-> which seemed needlessly noisy (diff-wise) and verbose.

Another thought on this: forcing the users of capture managers to be explicit about calling into the API capture manager or the common capture manager may help code clarity. For example, the calls to AcquireSharedApiCallLock are currently made on the API-specific capture manager classes D3D12CaptureManager or VulkanCaptureManager. This could make it not obvious that D3D12CaptureManager::AcquireSharedApiCallLock acquires a lock across all capture managers. Making the choice of capture manager explicit could help solidify the concept of global vs API-specific capture functions and promote correct usage.

Seems like this makes things more complicated. Especially because we're still sharing the file write locks. So now we'd be blocking on API calls and file write locks while the file write itself would be the mitigating restriction. So just having the file lock would be sufficient.

jzulauf-lunarg · 2024-05-06T16:01:35Z

framework/encode/d3d12_capture_manager.cpp

-    if (instance_->IsAnnotated() == true && instance_->resource_value_annotator_ == nullptr)
-    {
-        instance_->resource_value_annotator_ = std::make_unique<Dx12ResourceValueAnnotator>();
+    bool ret = CaptureManager::CreateInstance<D3D12CaptureManager>();


The templated overload associates the calling classes constructor/destructor functions automagically.

jzulauf-lunarg · 2024-05-06T16:09:36Z

@bradgrantham @davidd-lunarg -- I've added some explanatory comments. Please review so I can revise and complete to unblock the multi-api "work on a branch" there stuff that needs to be resolved after this to do multi-API correctly, but this will all forward progress, and should be functionally equivalent for the currently supported operating modes. (i.e. single API at a time)

framework/encode/capture_manager.cpp

ci-tester-lunarg · 2024-05-06T19:53:40Z

CI gfxreconstruct build queued with queue ID 180502.

ci-tester-lunarg · 2024-05-06T19:55:01Z

CI gfxreconstruct build # 4106 running.

ci-tester-lunarg · 2024-05-06T20:22:14Z

CI gfxreconstruct build # 4106 failed.

ci-tester-lunarg · 2024-05-06T20:39:49Z

CI gfxreconstruct build queued with queue ID 180524.

ci-tester-lunarg · 2024-05-06T20:40:01Z

CI gfxreconstruct build # 4107 running.

ci-tester-lunarg · 2024-05-06T21:08:00Z

CI gfxreconstruct build # 4107 failed.

ci-tester-lunarg · 2024-05-07T01:25:23Z

CI gfxreconstruct build queued with queue ID 180726.

ci-tester-lunarg · 2024-05-07T02:30:51Z

CI gfxreconstruct build # 4114 running.

ci-tester-lunarg · 2024-05-07T03:00:04Z

CI gfxreconstruct build # 4114 failed.

ci-tester-lunarg · 2024-05-07T14:52:45Z

CI gfxreconstruct build queued with queue ID 181045.

ci-tester-lunarg · 2024-05-07T14:55:01Z

CI gfxreconstruct build # 4115 running.

ci-tester-lunarg · 2024-05-07T15:20:32Z

CI gfxreconstruct build # 4115 failed.

ci-tester-lunarg · 2024-05-07T16:54:06Z

CI gfxreconstruct build queued with queue ID 181105.

ci-tester-lunarg · 2024-05-14T03:59:53Z

CI gfxreconstruct build # 4149 failed.

ci-tester-lunarg · 2024-05-14T17:14:03Z

CI gfxreconstruct build queued with queue ID 185214.

ci-tester-lunarg · 2024-05-14T17:45:22Z

CI gfxreconstruct build queued with queue ID 185233.

ci-tester-lunarg · 2024-05-14T18:23:22Z

CI gfxreconstruct build # 4154 running.

ci-tester-lunarg · 2024-05-14T18:57:23Z

CI gfxreconstruct build # 4154 passed.

jzulauf-lunarg · 2024-05-14T19:12:38Z

@davidd-lunarg @bradgrantham -- ready for review

framework/encode/api_capture_manager.cpp

MarkY-LunarG · 2024-05-14T20:27:08Z

framework/encode/capture_manager.h

+    using ApiCaptureManagerMap = std::unordered_map<ApiCaptureManager*, ApiInstanceRecord>;
+    ApiCaptureManagerMap api_capture_managers_;
+
+    CaptureSettings default_settings_;


You should clarify the difference here between default_settings_ and capture_settings_ in a comment.

framework/format/api_call_id.h

ci-tester-lunarg · 2024-05-14T21:04:53Z

CI gfxreconstruct build queued with queue ID 185371.

ci-tester-lunarg · 2024-05-14T21:10:01Z

CI gfxreconstruct build # 185371 cancelled.

ci-tester-lunarg · 2024-05-14T22:43:40Z

CI gfxreconstruct build queued with queue ID 185440.

ci-tester-lunarg · 2024-05-14T22:45:01Z

CI gfxreconstruct build # 4159 running.

ci-tester-lunarg · 2024-05-14T23:18:12Z

CI gfxreconstruct build # 4159 passed.

davidd-lunarg · 2024-05-14T23:40:56Z

framework/format/api_call_id.h

@@ -54,7 +54,7 @@ enum ApiFamilyId : uint16_t
    ApiFamily_Vulkan = 1,
    ApiFamily_Dxgi   = 2,
    ApiFamily_D3D12  = 3,
-    ApiFamily_AGS    = 4,
+    ApiFamily_AGS    = 4


Nit: this file doesn't need to be modified.

Agreed, okay to leave trailing comma (unless clang-format complained?)

bradgrantham-lunarg

Request to wrap loop in braces, echoed small nit on comma, otherwise looks great

bradgrantham-lunarg · 2024-05-15T00:47:37Z

framework/encode/capture_manager.cpp

 {
    capture_mode_ |= kModeWrite;

    auto thread_data = GetThreadData();
    assert(thread_data != nullptr);

-    WriteTrackedState(file_stream_.get(), thread_data->thread_id_);
+    for (auto& manager : api_capture_managers_)


Please wrap block in braces. Thank you!

bradgrantham-lunarg · 2024-05-15T00:49:57Z

framework/format/api_call_id.h

@@ -54,7 +54,7 @@ enum ApiFamilyId : uint16_t
    ApiFamily_Vulkan = 1,
    ApiFamily_Dxgi   = 2,
    ApiFamily_D3D12  = 3,
-    ApiFamily_AGS    = 4,
+    ApiFamily_AGS    = 4


Agreed, okay to leave trailing comma (unless clang-format complained?)

ci-tester-lunarg · 2024-05-15T16:05:51Z

CI gfxreconstruct build queued with queue ID 185949.

Split common and api specific options from isA to refsA relationship. Rename CaptureManager to CommonCaptureManager API specific capture managers inherit from ApiCaptureManager base, class. For single API sessions should be exactly equivalent.

ci-tester-lunarg · 2024-05-15T16:07:38Z

CI gfxreconstruct build queued with queue ID 185950.

ci-tester-lunarg · 2024-05-15T17:03:00Z

CI gfxreconstruct build # 4167 running.

ci-tester-lunarg · 2024-05-15T17:37:10Z

CI gfxreconstruct build # 4167 passed.

jzulauf-lunarg requested review from MarkY-LunarG, bradgrantham-lunarg and davidd-lunarg May 3, 2024 22:54

jzulauf-lunarg self-assigned this May 3, 2024

MarkY-LunarG reviewed May 3, 2024

View reviewed changes

framework/encode/capture_manager.h Outdated Show resolved Hide resolved

jzulauf-lunarg commented May 6, 2024

View reviewed changes

MarkY-LunarG reviewed May 6, 2024

View reviewed changes

framework/encode/capture_manager.cpp Show resolved Hide resolved

MarkY-LunarG reviewed May 6, 2024

View reviewed changes

framework/encode/capture_manager.cpp Show resolved Hide resolved

jzulauf-lunarg force-pushed the zulauf_breakup_capture_manager_singletons branch from f94320a to 2c5123c Compare May 6, 2024 19:53

jzulauf-lunarg force-pushed the zulauf_breakup_capture_manager_singletons branch from 2c5123c to 7ecaee7 Compare May 6, 2024 20:39

jzulauf-lunarg force-pushed the zulauf_breakup_capture_manager_singletons branch from 7ecaee7 to 48cb40c Compare May 7, 2024 14:52

jzulauf-lunarg force-pushed the zulauf_breakup_capture_manager_singletons branch from 48cb40c to 04c935d Compare May 7, 2024 16:54

jzulauf-lunarg force-pushed the zulauf_breakup_capture_manager_singletons branch from 24bc60b to b52fbf7 Compare May 14, 2024 17:14

jzulauf-lunarg marked this pull request as ready for review May 14, 2024 17:16

jzulauf-lunarg force-pushed the zulauf_breakup_capture_manager_singletons branch from b52fbf7 to a385ea6 Compare May 14, 2024 17:45

jzulauf-lunarg requested a review from MarkY-LunarG May 14, 2024 19:12

MarkY-LunarG reviewed May 14, 2024

View reviewed changes

framework/encode/api_capture_manager.cpp Outdated Show resolved Hide resolved

MarkY-LunarG reviewed May 14, 2024

View reviewed changes

framework/format/api_call_id.h Outdated Show resolved Hide resolved

jzulauf-lunarg force-pushed the zulauf_breakup_capture_manager_singletons branch from a385ea6 to 0d0cc69 Compare May 14, 2024 21:04

jzulauf-lunarg force-pushed the zulauf_breakup_capture_manager_singletons branch from 0d0cc69 to f1369cb Compare May 14, 2024 22:43

davidd-lunarg approved these changes May 15, 2024

View reviewed changes

bradgrantham-lunarg approved these changes May 15, 2024

View reviewed changes

jzulauf-lunarg force-pushed the zulauf_breakup_capture_manager_singletons branch from f1369cb to 4e6ee08 Compare May 15, 2024 16:05

jzulauf-lunarg force-pushed the zulauf_breakup_capture_manager_singletons branch from 4e6ee08 to 8b1bae3 Compare May 15, 2024 16:07

jzulauf-lunarg merged commit 3d93647 into LunarG:dev May 15, 2024
8 checks passed

Support multiple simultaneous API capture managers #1536

Support multiple simultaneous API capture managers #1536

Conversation

jzulauf-lunarg commented May 3, 2024

ci-tester-lunarg commented May 3, 2024

CLAassistant commented May 3, 2024 • edited Loading

ci-tester-lunarg commented May 3, 2024

ci-tester-lunarg commented May 3, 2024

jzulauf-lunarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidd-lunarg May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzulauf-lunarg commented May 6, 2024

ci-tester-lunarg commented May 6, 2024

ci-tester-lunarg commented May 6, 2024

ci-tester-lunarg commented May 6, 2024

ci-tester-lunarg commented May 6, 2024

ci-tester-lunarg commented May 6, 2024

ci-tester-lunarg commented May 6, 2024

ci-tester-lunarg commented May 7, 2024

ci-tester-lunarg commented May 7, 2024

ci-tester-lunarg commented May 7, 2024

ci-tester-lunarg commented May 7, 2024

ci-tester-lunarg commented May 7, 2024

ci-tester-lunarg commented May 7, 2024

ci-tester-lunarg commented May 7, 2024

ci-tester-lunarg commented May 14, 2024

ci-tester-lunarg commented May 14, 2024

ci-tester-lunarg commented May 14, 2024

ci-tester-lunarg commented May 14, 2024

ci-tester-lunarg commented May 14, 2024

jzulauf-lunarg commented May 14, 2024

MarkY-LunarG May 14, 2024 • edited Loading

Choose a reason for hiding this comment

ci-tester-lunarg commented May 14, 2024

ci-tester-lunarg commented May 14, 2024

ci-tester-lunarg commented May 14, 2024

ci-tester-lunarg commented May 14, 2024

ci-tester-lunarg commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradgrantham-lunarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-tester-lunarg commented May 15, 2024

ci-tester-lunarg commented May 15, 2024

ci-tester-lunarg commented May 15, 2024

ci-tester-lunarg commented May 15, 2024

CLAassistant commented May 3, 2024 •

edited

Loading

davidd-lunarg May 9, 2024 •

edited

Loading

MarkY-LunarG May 14, 2024 •

edited

Loading