Skip to content

Fix family queue selection in tests #448

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

Merged
axsaucedo merged 2 commits into from
Jan 29, 2026
Merged

Fix family queue selection in tests #448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7969663
checks if given family queue has compute capability
TinyTinni Jan 27, 2026
e35633f
test will search for queues with compute capabilities
TinyTinni Jan 27, 2026
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
16 changes: 16 additions & 0 deletions src/Manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,22 @@ Manager::createDevice(const std::vector<uint32_t>& familyQueueIndices,

this->mComputeQueueFamilyIndices.push_back(computeQueueFamilyIndex);
} else {
std::vector<vk::QueueFamilyProperties> allQueueFamilyProperties =
physicalDevice.getQueueFamilyProperties();
for (auto queueIndexGiven : familyQueueIndices) {
if (queueIndexGiven >= allQueueFamilyProperties.size()) {
throw std::runtime_error(
"Given family queue index does not exists. Index given: " +
std::to_string(queueIndexGiven));
}
if (!(allQueueFamilyProperties[queueIndexGiven].queueFlags &
vk::QueueFlagBits::eCompute)) {
throw std::runtime_error(
"Given family queue index does not support compute "
"operations. Index given: " +
std::to_string(queueIndexGiven));
}
}
this->mComputeQueueFamilyIndices = familyQueueIndices;
}

Expand Down
43 changes: 37 additions & 6 deletions test/TestAsyncOperations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,32 @@
#include "kompute/logger/Logger.hpp"
#include "shaders/Utils.hpp"

namespace {
std::vector<uint32_t>
distinctFamilyQueueIndices(const vk::PhysicalDevice& device)
{
const std::vector<vk::QueueFamilyProperties> allQueueFamilyProperties =
device.getQueueFamilyProperties();
std::vector<uint32_t> distinctQueuesIndices;

for (uint32_t i = 0; i < allQueueFamilyProperties.size(); i++) {
if (allQueueFamilyProperties[i].queueFlags &
(vk::QueueFlagBits::eCompute)) {
distinctQueuesIndices.push_back(i);
}
}
return distinctQueuesIndices;
}
}

TEST(TestAsyncOperations, TestManagerParallelExecution)
{
// This test is built for NVIDIA 1650. It assumes:
// * Queue family 0 and 2 have compute capabilities
// This test assumes:
// * There are at least 2 different Queue families with compute capabilities
// * GPU is able to process parallel shader code across different families
Copy link
Member

@axsaucedo axsaucedo Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires not only compute enabled queues, but it actually requres parallel enabled queues. This is something not all GPUs support, would be interesting if there's a way to actually check for parallel enabled queues

Copy link
Contributor Author

@TinyTinni TinyTinni Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean GPUs which have the second implementation referenced in the vulkan doc for the queues?

How a VkQueue is mapped to the underlying hardware is implementation-defined. Some implementations will have multiple hardware queues and submitting work to multiple VkQueue​s will proceed independently and concurrently. Some implementations will do scheduling at a kernel driver level before submitting work to the hardware. There is no current way in Vulkan to expose the exact details how each VkQueue is mapped.

(emphasize mine) source: https://docs.vulkan.org/guide/latest/queues.html

Copy link
Member

@axsaucedo axsaucedo Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - I mean queues that can actually run workloads in parallel (ref https://towardsdatascience.com/parallelizing-heavy-gpu-workloads-via-multi-queue-operations-50a38b15a1dc)

Copy link
Contributor Author

@TinyTinni TinyTinni Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see the problem. If the queues are not executed in parallel, the test can fail because of the failed timing measurement.

kompute/test/TestAsyncOperations.cpp

Lines 120 to 121 in ec7f816

// The speedup should be at least 40%
EXPECT_LT(durationAsync, durationSync * 0.6);

I didn't expect timing measurement in a test. Wouldn't it be better to declare this test as a benchmark? Or copy it and just add this assert to the benchmark. Logically, the test should run fine even on hardware with just serial execution, except that it is probably not that much faster, which doesn't invalidate the correctness.

Copy link
Member

@axsaucedo axsaucedo Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct - indeed it's commented explicitly in the test. I think it'd be valid to mark it as benchmark, but indeed it's not a bug as initially assumed.

We have a benchmark section that it can be moved to - would also be interesting if its possible to identify parallel enabled queues, but previously i had to look at GPU manuals to find out which are the parallel supported queues.

Copy link
Contributor Author

@TinyTinni TinyTinni Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is currently no way on how to get this information. There is some discussion on the vulkan github [1] about it, but it is from 2020 and it is probably not going forward.

What do you think, should we move the whole test to benchmark or alternatively, we add at least a message to the assert. When it fails, at least there will be an explanation to the user on why it fails and that maybe his hardware does not support parallel execution (or the 2 specific queues cannot run in parallel, which can also happen on AMD systems when they are queued next to each other, like someone mentioned in [1]).

Something like

EXPECT_LT(durationAsync, durationSync * 0.6) << "There was no speedup in using multiple queues from different QueueFamilies. Maybe your GPU does not support parallel execution.";

[1] KhronosGroup/Vulkan-Docs#569

Copy link
Member

@axsaucedo axsaucedo Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that sounds like a good suggestion!

Copy link
Contributor Author

@TinyTinni TinyTinni Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the message to the assertion.

uint32_t size = 10;
constexpr uint32_t size = 10;

uint32_t numParallel = 2;
constexpr uint32_t numParallel = 2;

std::string shader(R"(
#version 450
Expand Down Expand Up @@ -79,7 +97,18 @@ TEST(TestAsyncOperations, TestManagerParallelExecution)
EXPECT_EQ(inputsSyncB[i]->vector<float>(), resultSync);
}

kp::Manager mgrAsync(0, { 0, 2 });
constexpr uint32_t deviceId =
0u; // device 0 exists, because "mgr" could be created already
auto queues = distinctFamilyQueueIndices(
mgr.getVkInstance()->enumeratePhysicalDevices().at(deviceId));
if (queues.size() < numParallel) {
GTEST_SKIP() << "GPU does not support multiple compute queues. Only "
<< queues.size() << " are supported. Skipping test.";
}

queues.resize(numParallel);

kp::Manager mgrAsync(deviceId, std::move(queues));

std::vector<std::shared_ptr<kp::Memory>> inputsAsyncB;

Expand Down Expand Up @@ -118,7 +147,9 @@ TEST(TestAsyncOperations, TestManagerParallelExecution)
}

// The speedup should be at least 40%
EXPECT_LT(durationAsync, durationSync * 0.6);
EXPECT_LT(durationAsync, durationSync * 0.6)
<< "There was no speedup in using multiple queues from different "
"QueueFamilies. Maybe your GPU does not support parallel execution.";
}

TEST(TestAsyncOperations, TestManagerAsyncExecution)
Expand Down