-
Notifications
You must be signed in to change notification settings - Fork 82
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.
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
base: main
Are you sure you want to change the base?
Conversation
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
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 😅
src/VkBootstrap.h
Outdated
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; |
There was a problem hiding this comment.
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
While looking at the code, I've noticed that a lot of functions return a plain
std::vector
not behind a reference, that creates extra copying that could be avoided. I've also moved some code from the cpp file to the header file (like theCustomQueueDescription
constructor) so that the compiler can inline the code. I think the whole of vk-bootstrap could benefit from that. I should make a separate issue about this.Also I'm not super sure about the formatting, I did format it with clang-format, but stuff like pointer alignment wasn't changed.
Fixes the should've-been-a-new-issue #224