Skip to content
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

Improve function std::vector arguments, add C++20 std::span support #268

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Improve function std::vector arguments, add C++20 std::span support #268

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1fa2b08
Improve function std::vector arguments, add C++20 std::span support
monomere Mar 29, 2024
c24b341
Clean up the header file
monomere Mar 29, 2024
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
36 changes: 9 additions & 27 deletions src/VkBootstrap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,12 +768,6 @@ InstanceBuilder& InstanceBuilder::enable_extension(const char* extension_name) {
info.extensions.push_back(extension_name);
return *this;
}
InstanceBuilder& InstanceBuilder::enable_extensions(std::vector<const char*> const& extensions) {
for (const auto extension : extensions) {
info.extensions.push_back(extension);
}
return *this;
}
InstanceBuilder& InstanceBuilder::enable_extensions(size_t count, const char* const* extensions) {
if (!extensions || count == 0) return *this;
for (size_t i = 0; i < count; i++) {
Expand Down Expand Up @@ -1038,7 +1032,7 @@ PhysicalDevice PhysicalDeviceSelector::populate_device_details(VkPhysicalDevice
} else {
detail::vulkan_functions().fp_vkGetPhysicalDeviceFeatures2KHR(vk_phys_device, &local_features);
}
physical_device.extended_features_chain = fill_chain;
physical_device.extended_features_chain = std::move(fill_chain);
}

return physical_device;
Expand Down Expand Up @@ -1299,12 +1293,6 @@ PhysicalDeviceSelector& PhysicalDeviceSelector::add_required_extension(const cha
criteria.required_extensions.push_back(extension);
return *this;
}
PhysicalDeviceSelector& PhysicalDeviceSelector::add_required_extensions(std::vector<const char*> const& extensions) {
for (const auto& ext : extensions) {
criteria.required_extensions.push_back(ext);
}
return *this;
}
PhysicalDeviceSelector& PhysicalDeviceSelector::add_required_extensions(size_t count, const char* const* extensions) {
if (!extensions || count == 0) return *this;
for (size_t i = 0; i < count; i++) {
Expand Down Expand Up @@ -1399,15 +1387,16 @@ bool PhysicalDevice::enable_extension_if_present(const char* extension) {
}
return false;
}
bool PhysicalDevice::enable_extensions_if_present(const std::vector<const char*>& extensions) {
for (const auto extension : extensions) {
bool PhysicalDevice::enable_extensions_if_present(size_t count, const char* const* extensions) {
for (size_t i = 0; i < count; ++i) {
const auto extension = extensions[i];
auto it = std::find_if(std::begin(available_extensions),
std::end(available_extensions),
[extension](std::string const& ext_name) { return ext_name == extension; });
if (it == std::end(available_extensions)) return false;
}
for (const auto extension : extensions)
extensions_to_enable.push_back(extension);
for (size_t i = 0; i < count; ++i)
extensions_to_enable.push_back(extensions[i]);
return true;
}

Expand Down Expand Up @@ -1479,9 +1468,6 @@ DispatchTable Device::make_table() const { return { device, fp_vkGetDeviceProcAd

Device::operator VkDevice() const { return this->device; }

CustomQueueDescription::CustomQueueDescription(uint32_t index, std::vector<float> priorities)
: index(index), priorities(std::move(priorities)) {}

void destroy_device(Device const& device) {
device.internal_table.fp_vkDestroyDevice(device.device, device.allocation_callbacks);
}
Expand Down Expand Up @@ -1584,10 +1570,6 @@ Result<Device> DeviceBuilder::build() const {
device.instance_version = physical_device.instance_version;
return device;
}
DeviceBuilder& DeviceBuilder::custom_queue_setup(std::vector<CustomQueueDescription> queue_descriptions) {
info.queue_descriptions = std::move(queue_descriptions);
return *this;
}
DeviceBuilder& DeviceBuilder::set_allocation_callbacks(VkAllocationCallbacks* callbacks) {
info.allocation_callbacks = callbacks;
return *this;
Expand Down Expand Up @@ -1938,9 +1920,9 @@ Result<std::vector<VkImageView>> Swapchain::get_image_views(const void* pNext) {
}
return views;
}
void Swapchain::destroy_image_views(std::vector<VkImageView> const& image_views) {
for (auto& image_view : image_views) {
internal_table.fp_vkDestroyImageView(device, image_view, allocation_callbacks);
void Swapchain::destroy_image_views(size_t count, VkImageView const* image_views) {
for (size_t i = 0; i < count; ++i) {
internal_table.fp_vkDestroyImageView(device, image_views[i], allocation_callbacks);
}
}
Swapchain::operator VkSwapchainKHR() const { return this->swapchain; }
Expand Down
101 changes: 94 additions & 7 deletions src/VkBootstrap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,16 @@
#include <cstdio>
#include <cstring>

#if __cplusplus >= 202002L
#define VKB_SPAN_OVERLOADS 1
#elif !defined(VKB_SPAN_OVERLOADS)
#define VKB_SPAN_OVERLOADS 0
#endif

#if VKB_SPAN_OVERLOADS
#include <span>
#endif

#include <vector>
#include <string>
#include <system_error>
Expand Down Expand Up @@ -376,9 +386,22 @@ class InstanceBuilder {
InstanceBuilder& enable_layer(const char* layer_name);
// Adds an extension to be enabled. Will fail to create an instance if the extension isn't available.
InstanceBuilder& enable_extension(const char* extension_name);
InstanceBuilder& enable_extensions(std::vector<const char*> const& extensions);

// Add extensions to be enabled. Will fail to create an instance if the extension aren't available.
InstanceBuilder& enable_extensions(size_t count, const char* const* extensions);

// Add extensions to be enabled. Will fail to create an instance if the extension aren't available.
InstanceBuilder& enable_extensions(std::vector<const char*> const& extensions) {
return enable_extensions(extensions.size(), extensions.data());
}

#if VKB_SPAN_OVERLOADS
// Add extensions to be enabled. Will fail to create an instance if the extension aren't available.
InstanceBuilder& enable_extensions(std::span<const char*> extensions) {
return enable_extensions(extensions.size(), extensions.data());
}
#endif
Comment on lines +394 to +403
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we have the implementation for the vector & span functions that just forward the arguments to the count + pointer function while the implementation of the count + pointer function is in the .cpp file?

I would prefer it if all of the implementations were in the .cpp file. While I do think inlining is important, I would much rather make any performance based decision on data, not conjecture.

Copy link
Author

Choose a reason for hiding this comment

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

To answer the first question: so that less code is duplicated

iirc LTO isn't enabled by default on gcc nor clang, which is generally the only way for the compiler to be able to optimize these functions if they weren't inline. Also afaik and it is common practice to put small forwarding functions and simple constructors in the class definition. I could make a small benchmark for this even :)

Copy link
Author

Choose a reason for hiding this comment

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

Although if you really prefer the function bodies to be in the .cpp file (even if they're a single line), I can change the code

Also I think a great possible performance improvement is the operator VkXX methods. It would be the difference between calling a small function (moving stack pointer, etc.) that returns a pointer and using the pointer directly. And since these operators could be used almost everywhere in user code (like passing Device or Instance) it has a measurable impact I'd say.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes I really would like to see benchmarks - because vkb is an initialization library, very little of this code is called more than once in an application. This library definitely prefers maintainability over performance, and keeping implementations out of the .h file is one way to aid that - as the .h file is in effect the 'documentation'.

Copy link
Author

@monomere monomere Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The header files being a sort of documentation is a fair point! (this could be fixed by moving the implementation outside of the class definition, but still in the header file though)

I'll move the method bodies into the .cpp file for now. Although I would still suggest moving the operator VkXX methods into the header (it doesn't clutter the code that much, since it fits completely on one line, even with the method declaration)

I'll make the benchmark a bit later, since its not as simple as benchmarking pointer access. But here's a simple godbolt disassembly (not a whole lot of instructions, so its easy to see the point). Compiled with -O2.

extern void do_stuff(int);
void test(A& a) { // A { int unused; int *p; }
    do_stuff(*a.fn1()); // declared out-of-line
    do_stuff(*a.fn2()); // declared inline, returns this->p.
}
test(A&):
  push    rbx
  mov     rbx, rdi
  
  ; this is the out-of-line
  call    A::fn1() ; compiles to "mov rax, qword ptr [rdi+8]; ret"
  mov     edi, DWORD PTR [rax]
  call    do_stuff(int)
  
  ; this is inline
  mov     rax, QWORD PTR [rbx+8]
  mov     edi, DWORD PTR [rax]
  call    do_stuff(int)

So even by just analyzing the assembly, you could deduce that when this is being used in a context of another function invocation (most likely vkXX), the compiler would have to save a lot of registers that store the function arguments, and invoke another function which is pointless indirection.
Edit: make the wording a bit more clear 😅


// Headless Mode does not load the required extensions for presentation. Defaults to true.
InstanceBuilder& set_headless(bool headless = true);

Expand Down Expand Up @@ -510,7 +533,16 @@ struct PhysicalDevice {

// If all the given extensions are present, make all the extensions be enabled on the device.
// Returns true if all the extensions are present.
bool enable_extensions_if_present(const std::vector<const char*>& extensions);
bool enable_extensions_if_present(size_t count, const char* const* extensions);
bool enable_extensions_if_present(const std::vector<const char*>& extensions) {
return enable_extensions_if_present(extensions.size(), extensions.data());
}

#if VKB_SPAN_OVERLOADS
bool enable_extensions_if_present(std::span<const char*> extensions) {
return enable_extensions_if_present(extensions.size(), extensions.data());
}
#endif

// A conversion function which allows this PhysicalDevice to be used
// in places where VkPhysicalDevice would have been used.
Expand Down Expand Up @@ -595,8 +627,17 @@ class PhysicalDeviceSelector {
// Require a physical device which supports a specific extension.
PhysicalDeviceSelector& add_required_extension(const char* extension);
// Require a physical device which supports a set of extensions.
PhysicalDeviceSelector& add_required_extensions(std::vector<const char*> const& extensions);
PhysicalDeviceSelector& add_required_extensions(size_t count, const char* const* extensions);
PhysicalDeviceSelector& add_required_extensions(std::vector<const char*> const& extensions) {
return add_required_extensions(extensions.size(), extensions.data());
}

#if VKB_SPAN_OVERLOADS
// Require a physical device which supports a set of extensions.
PhysicalDeviceSelector& add_required_extensions(std::span<const char*> extensions) {
return add_required_extensions(extensions.size(), extensions.data());
}
#endif

// Prefer a physical device which supports a specific extension.
[[deprecated("Use vkb::PhysicalDevice::enable_extension_if_present instead")]] PhysicalDeviceSelector&
Expand Down Expand Up @@ -741,8 +782,21 @@ struct Device {

// For advanced device queue setup
struct CustomQueueDescription {
explicit CustomQueueDescription(uint32_t index, std::vector<float> priorities);
uint32_t index = 0;
explicit CustomQueueDescription(uint32_t index, std::vector<float> const& priorities)
: index(index), priorities(priorities) {}

explicit CustomQueueDescription(uint32_t index, std::vector<float>&& priorities)
: index(index), priorities(std::move(priorities)) {}

explicit CustomQueueDescription(uint32_t index, size_t count, float const* priorities)
: index(index), priorities(priorities, priorities + count) {}

#if VKB_SPAN_OVERLOADS
explicit CustomQueueDescription(uint32_t index, std::span<const float> priorities)
: index(index), priorities(priorities.begin(), priorities.end()) {}
#endif

uint32_t index;
std::vector<float> priorities;
};

Expand All @@ -757,7 +811,31 @@ class DeviceBuilder {

// For Advanced Users: specify the exact list of VkDeviceQueueCreateInfo's needed for the application.
// If a custom queue setup is provided, getting the queues and queue indexes is up to the application.
DeviceBuilder& custom_queue_setup(std::vector<CustomQueueDescription> queue_descriptions);
DeviceBuilder& custom_queue_setup(size_t count, CustomQueueDescription const* queue_descriptions) {
info.queue_descriptions.assign(queue_descriptions, queue_descriptions + count);
return *this;
}
// For Advanced Users: specify the exact list of VkDeviceQueueCreateInfo's needed for the application.
// If a custom queue setup is provided, getting the queues and queue indexes is up to the application.
DeviceBuilder& custom_queue_setup(std::vector<CustomQueueDescription> const& queue_descriptions) {
info.queue_descriptions = queue_descriptions;
return *this;
}
// For Advanced Users: specify the exact list of VkDeviceQueueCreateInfo's needed for the application.
// If a custom queue setup is provided, getting the queues and queue indexes is up to the application.
DeviceBuilder& custom_queue_setup(std::vector<CustomQueueDescription>&& queue_descriptions) {
info.queue_descriptions = std::move(queue_descriptions);
return *this;
}

#if VKB_SPAN_OVERLOADS
// For Advanced Users: specify the exact list of VkDeviceQueueCreateInfo's needed for the application.
// If a custom queue setup is provided, getting the queues and queue indexes is up to the application.
DeviceBuilder& custom_queue_setup(std::span<const CustomQueueDescription> queue_descriptions) {
info.queue_descriptions.assign(queue_descriptions.begin(), queue_descriptions.end());
return *this;
Copy link
Owner

Choose a reason for hiding this comment

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

Same as the InstanceBuilder, would much prefer implementations to be in the .cpp

}
#endif

// Add a structure to the pNext chain of VkDeviceCreateInfo.
// The structure must be valid when DeviceBuilder::build() is called.
Expand Down Expand Up @@ -802,7 +880,16 @@ struct Swapchain {
// structure.
Result<std::vector<VkImageView>> get_image_views();
Result<std::vector<VkImageView>> get_image_views(const void* pNext);
void destroy_image_views(std::vector<VkImageView> const& image_views);
void destroy_image_views(size_t count, VkImageView const* image_views);
void destroy_image_views(std::vector<VkImageView> const& image_views) {
destroy_image_views(image_views.size(), image_views.data());
}

#if VKB_SPAN_OVERLOADS
void destroy_image_views(std::span<const VkImageView> image_views) {
destroy_image_views(image_views.size(), image_views.data());
}
charles-lunarg marked this conversation as resolved.
Show resolved Hide resolved
#endif

// A conversion function which allows this Swapchain to be used
// in places where VkSwapchainKHR would have been used.
Expand Down