Marky update loop event handling #1860

MarkY-LunarG · 2024-11-01T19:59:15Z

No description provided.

Update the logic to skip the unknown event type (which is not valid) and also use the MAX enum value so it never gets actually hit.

Update the "received_events_" variable to the name of "previously_received_unhandled_events_" to be clearer.

Update how many events are stripped from the list of previously received events from 100 to 300 so that we don't keep hitting it as often.

Add the initial framework for tracking events and behaving differently for different events.

If we fail to encounter an event during a loop, reduce the future time we wait for it. But, only if we allow the reduction of that.

Assert on a missed event if we're supposed to, but not if we're ok with missing the event.

Add some clearer comments and log messages.

ci-tester-lunarg · 2024-11-01T19:59:18Z

CI gfxreconstruct build queued with queue ID 292992.

ci-tester-lunarg · 2024-11-01T20:00:00Z

CI gfxreconstruct build # 5266 running.

ci-tester-lunarg · 2024-11-01T20:27:32Z

CI gfxreconstruct build queued with queue ID 293015.

We don't want to increase the sleep time. That would mean we wait longer than we want for previously missed events. Only reduce the retry limit so we don't get the longer sleep times.

ci-tester-lunarg · 2024-11-01T21:12:59Z

CI gfxreconstruct build queued with queue ID 293060.

MarkY-LunarG · 2024-11-01T21:13:09Z

Accidentally included a submodule update in last run. Fixed.

jzulauf-lunarg

This is much nicer, a couple nits, a functional question, and a storage question or two.

external/Vulkan-Headers

framework/decode/openxr_replay_consumer_base.cpp

jzulauf-lunarg · 2024-11-01T20:59:04Z

framework/decode/openxr_replay_consumer_base.cpp

@@ -826,15 +848,15 @@ void OpenXrReplayConsumerBase::Process_xrPollEvent(const ApiCallInfo&
    XrEventDataBuffer* capture_event = eventData->GetPointer();

    // We received events that haven't been handled yet already, so see if this one is in the list already
-    for (size_t ii = 0; ii < received_events_.size(); ++ii)
+    for (size_t ii = 0; ii < previously_received_unhandled_events_.size(); ++ii)


I didn't realize that size and [] are O(1) for std::deque. The erase will be O(n) since you are potentially shuffling within the blocks that.

Linked list std::list would be O(1) per-item inserted/deleted for insert/delete as is size (since '11), but you'd need to use iterators in the for loop.

jzulauf-lunarg · 2024-11-01T21:03:19Z

framework/decode/openxr_replay_consumer_base.cpp

        {
-            GFXRECON_LOG_WARNING("Previously received event %s (0x%x, %u)",
+            GFXRECON_LOG_WARNING("Previously received unhandled event %s (0x%x, %u)",


INFO "Using previously received event of type ..." ?

jzulauf-lunarg · 2024-11-01T21:21:04Z

framework/decode/openxr_replay_consumer_base.cpp

+    // If we have not previously started tracking the event behavior for this event type, start tracking it.
+    if (event_behavior_tracking_.find(capture_event->type) == event_behavior_tracking_.end())
+    {
+        event_behavior_tracking_[capture_event->type] = EventBehaviorTracking();


std::unordered_map::operator[] automatically inserts a default initialized entry, when key is not found. So this block is unneeded.

What would be useful instead is a reference EventBehaviorTracking &behavior_tracking = event_behavior_tracking_[capture_event->type] given that '[]' is best O(1) is worst case O(n). and that same dereference is done many times below.

I didn't know that about the initialize. Thanks.

jzulauf-lunarg · 2024-11-01T21:29:55Z

framework/decode/openxr_replay_consumer_base.cpp

+            uint32_t current_reduction = event_behavior_tracking_[capture_event->type].current_timeout_reduction;
+            if (current_reduction >= retry_limit)
+            {
+                retry_limit = 0;


Needs to be one, or we won't even look for the event once. We're not going to drop anything, as we save anything other that we get in previously_received_unhandled_events_ (unless we think there's going to be a storm of missing events)

jzulauf-lunarg · 2024-11-01T21:31:00Z

framework/decode/openxr_replay_consumer_base.cpp

-                    received_events_.push_back(*out_eventData);
+                    // If this was a valid event, but not the one we're interested in, record
+                    // it to a list of received events for us to use later.
+                    previously_received_unhandled_events_.push_back(*out_eventData);
                    GFXRECON_LOG_WARNING("Recording event for later %s (0x%x, %u)",


jzulauf-lunarg · 2024-11-01T21:31:42Z

framework/decode/openxr_replay_consumer_base.cpp

                    // events to make room for more without bloating the list size.
                    // TODO: Perhaps do this more elegantly?
-                    if (received_events_.size() > 1000)
+                    if (previously_received_unhandled_events_.size() > 1000)


1000 and 300 should be const statics of the ReplayConsumer

and maybe like 5000, and 500?

I switched to constants. And I set them to 2000 and 500. I don't like it to get too big because they are large structures.

framework/decode/openxr_replay_consumer_base.cpp

jzulauf-lunarg · 2024-11-01T21:41:26Z

framework/decode/openxr_replay_consumer_base.h

@@ -484,7 +484,7 @@ class OpenXrReplayConsumerBase : public OpenXrConsumer

    VulkanReplayConsumerBase* vulkan_replay_consumer_ = nullptr;

-    std::vector<XrEventDataBuffer> received_events_;
+    std::deque<XrEventDataBuffer> previously_received_unhandled_events_;


XrEvenDataBuffer are large, and maybe shuffled. Do we want to store a unique_ptr to them instead (or use std::list to ensure no shuffling occurs)?

Using a list now based on your suggestion.

ci-tester-lunarg · 2024-11-01T21:53:18Z

CI gfxreconstruct build # 5270 running.

ci-tester-lunarg · 2024-11-01T22:33:13Z

CI gfxreconstruct build # 5270 passed.

jzulauf-lunarg

One more little nit.

jzulauf-lunarg · 2024-11-01T23:58:27Z

framework/decode/openxr_replay_consumer_base.h

+    {
+        // Do we allow the event to waited on less and less each time we fail to
+        // properly receive the event?
+        bool allow_timeout_reduction{ true };


for better packing int, bool, bool

jzulauf-lunarg · 2024-11-04T16:34:25Z

framework/decode/openxr_replay_consumer_base.cpp

-    Decoded_XrSessionCreateInfo* decoded_info   = createInfo->GetMetaStructPointer();
-    SessionData&                 session_data   = AddSessionData(*session->GetPointer());
+    Decoded_XrSessionCreateInfo* decoded_info = createInfo->GetMetaStructPointer();
+    SessionData&                 session_data = AddSessionData(*session->GetPointer());


Intentional white space change?

I thought I reverted this. My clang changed it. I'll revert again.

ci-tester-lunarg · 2024-11-04T20:51:03Z

CI gfxreconstruct build queued with queue ID 295086.

ci-tester-lunarg · 2024-11-04T20:55:00Z

CI gfxreconstruct build # 5284 running.

ci-tester-lunarg · 2024-11-04T21:27:53Z

CI gfxreconstruct build # 5284 passed.

jzulauf-lunarg

LGTM

MarkY-LunarG added 8 commits November 1, 2024 15:36

openxr: Update GetEventTypeString

f50f6ae

Update the logic to skip the unknown event type (which is not valid) and also use the MAX enum value so it never gets actually hit.

openxr: Update received_events_ variable name

5cbe2ef

Update the "received_events_" variable to the name of "previously_received_unhandled_events_" to be clearer.

openxr: Update event stripping

236d7b0

Update how many events are stripped from the list of previously received events from 100 to 300 so that we don't keep hitting it as often.

openxr: Add initial event tracking

c60953c

Add the initial framework for tracking events and behaving differently for different events.

openxr: Start using the event tracking data

2eb551b

If we fail to encounter an event during a loop, reduce the future time we wait for it. But, only if we allow the reduction of that.

openxr: Assert only if we're supposed to

677526c

Assert on a missed event if we're supposed to, but not if we're ok with missing the event.

openxr: Add some clearer comments

effc706

Add some clearer comments and log messages.

openxr: Clang-format diffs versus current tree

1d48a4f

MarkY-LunarG requested a review from jzulauf-lunarg November 1, 2024 19:59

openxr: Fix sleep time

0e7d890

We don't want to increase the sleep time. That would mean we wait longer than we want for previously missed events. Only reduce the retry limit so we don't get the longer sleep times.

MarkY-LunarG added the openxr-experimental Related to OpenXR experimental support label Nov 1, 2024

MarkY-LunarG mentioned this pull request Nov 1, 2024

openxr: Unknown events in capture but not replay #1787

Closed

MarkY-LunarG force-pushed the marky-update-loop-event-handling branch from b370cc6 to 0e7d890 Compare November 1, 2024 21:12

jzulauf-lunarg reviewed Nov 1, 2024

View reviewed changes

jzulauf-lunarg reviewed Nov 4, 2024

View reviewed changes

Apply code review comments

ba56c2d

jzulauf-lunarg approved these changes Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marky update loop event handling #1860

Marky update loop event handling #1860

MarkY-LunarG commented Nov 1, 2024

ci-tester-lunarg commented Nov 1, 2024

ci-tester-lunarg commented Nov 1, 2024

ci-tester-lunarg commented Nov 1, 2024

ci-tester-lunarg commented Nov 1, 2024

MarkY-LunarG commented Nov 1, 2024

jzulauf-lunarg left a comment

jzulauf-lunarg Nov 1, 2024 •

edited

Loading

jzulauf-lunarg Nov 1, 2024

jzulauf-lunarg Nov 1, 2024

MarkY-LunarG Nov 4, 2024

jzulauf-lunarg Nov 1, 2024

jzulauf-lunarg Nov 1, 2024

jzulauf-lunarg Nov 1, 2024

jzulauf-lunarg Nov 1, 2024

MarkY-LunarG Nov 4, 2024

jzulauf-lunarg Nov 1, 2024

MarkY-LunarG Nov 4, 2024

ci-tester-lunarg commented Nov 1, 2024

ci-tester-lunarg commented Nov 1, 2024

jzulauf-lunarg left a comment

jzulauf-lunarg Nov 1, 2024

jzulauf-lunarg Nov 4, 2024

MarkY-LunarG Nov 4, 2024

ci-tester-lunarg commented Nov 4, 2024

ci-tester-lunarg commented Nov 4, 2024

ci-tester-lunarg commented Nov 4, 2024

jzulauf-lunarg left a comment

Marky update loop event handling #1860

Are you sure you want to change the base?

Marky update loop event handling #1860

Conversation

MarkY-LunarG commented Nov 1, 2024

ci-tester-lunarg commented Nov 1, 2024

ci-tester-lunarg commented Nov 1, 2024

ci-tester-lunarg commented Nov 1, 2024

ci-tester-lunarg commented Nov 1, 2024

MarkY-LunarG commented Nov 1, 2024

jzulauf-lunarg left a comment

Choose a reason for hiding this comment

jzulauf-lunarg Nov 1, 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

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

ci-tester-lunarg commented Nov 1, 2024

ci-tester-lunarg commented Nov 1, 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

ci-tester-lunarg commented Nov 4, 2024

ci-tester-lunarg commented Nov 4, 2024

ci-tester-lunarg commented Nov 4, 2024

jzulauf-lunarg left a comment

Choose a reason for hiding this comment

jzulauf-lunarg Nov 1, 2024 •

edited

Loading