KhronosGroup / Vulkan-ValidationLayers

Vulkan Validation Layers (VVL)
https://vulkan.lunarg.com/doc/sdk/latest/linux/khronos_validation_layer.html
Other
763 stars 403 forks source link

BestPractices-SyncObjects-HighNumberOfFences : off by one error #8469

Open filnet opened 2 months ago

filnet commented 2 months ago

Environment:

Describe the Issue

The BestPractices-SyncObjects-HighNumberOfFences warning triggers after creating the fifth fence and not after creating the fourth (the limit is defined as 3). The check is done in a PreCallValidateCreateFence so maybe before the count is updated...

Side note: would be nice if the warning message included the limit and the current count.

PS: other such checks might have the same issue.

filnet commented 2 months ago

This check seems a bit hard to not trigger as fences are not used only for in flight frames... I allow only two in flight frames so have only 2 fences for that. But I also use fences for waiting for queued commands to finish executing. So currently I have 7 fences and trigger this warning.

spencer-lunarg commented 2 months ago

Made a PR to fix the error message, not fully sure if there is threading issues or what, but the VkAmdBestPracticesLayerTest.NumSyncPrimitives test shows we report on the 4th fence there

filnet commented 2 months ago

My code is not multi threaded during the initialization phase. I added a debug printf prior to every call to create_fence and the output shows that the warning is emitted on the 5th call and thereafter. The warning is emmitted 3 times for 7 created fences. So one short.

    Line 111: CREATE FENCE
    Line 113: CREATE FENCE
    Line 193: CREATE FENCE
    Line 234: CREATE FENCE
    Line 377: CREATE FENCE
    Line 378: [Warning][Performance]"Validation Performance Warning: [ BestPractices-SyncObjects-HighNumberOfFences ] | MessageID = 0x
    Line 379: a9f4ff68 | vkCreateFence():  [AMD] [NVIDIA] High number of VkFence objects created.Minimize the amount of CPU-GPU synchr
    Line 380: onization that is used. Semaphores and fences have overhead. Each fence has a CPU and GPU cost with it."
    Line 381: CREATE FENCE
    Line 382: [Warning][Performance]"Validation Performance Warning: [ BestPractices-SyncObjects-HighNumberOfFences ] | MessageID = 0x
    Line 383: a9f4ff68 | vkCreateFence():  [AMD] [NVIDIA] High number of VkFence objects created.Minimize the amount of CPU-GPU synchr
    Line 384: onization that is used. Semaphores and fences have overhead. Each fence has a CPU and GPU cost with it."
    Line 388: CREATE FENCE
    Line 389: [Warning][Performance]"Validation Performance Warning: [ BestPractices-SyncObjects-HighNumberOfFences ] | MessageID = 0x
    Line 390: a9f4ff68 | vkCreateFence():  [AMD] [NVIDIA] High number of VkFence objects created.Minimize the amount of CPU-GPU synchr
    Line 391: onization that is used. Semaphores and fences have overhead. Each fence has a CPU and GPU cost with it."

EDIT: I also added debug printf when destroying fences. All seven fences are destroyed when exiting (i.e. there are no short lived fences in the initialization phase).

filnet commented 2 months ago

My hunch is that the hook that checks the number of created fences is called before the hook that increments the count.

filnet commented 2 months ago

And the test expects the warning on the 5th fence creation (and not the 4th as desired)..

The loop creates 4 fences and then a fifth is created to trigger the warning:

    constexpr int fence_warn_limit = 5;
    const auto fence_ci = vkt::Fence::create_info();
    std::vector<vkt::Fence> test_fences(fence_warn_limit);
    for (int i = 0; i < fence_warn_limit - 1; ++i) {
        test_fences[i].init(*m_device, fence_ci);
    }
    m_errorMonitor->SetDesiredFailureMsg(kPerformanceWarningBit, "BestPractices-SyncObjects-HighNumberOfFences");
    test_fences[fence_warn_limit - 1].init(*m_device, fence_ci);
    m_errorMonitor->VerifyFound();

fence_warn_limit should be set to kMaxRecommendedFenceObjectsSizeAMD + 1 (i.e. 4)