Improved safe_struct #120
Description
We currently have adopted Vulkan Utility Libraries safe-struct for making modifiable copies of input structures and their pNext chains. This is functional, but has some scope for improvements if we want to improve the efficiency and type-safety of the library.
The main issue is that VUL still needs some uses of const_cast in the layer code, even though it's not really needed. Solving that is the highest priority here, other issues are just logged here for future consideration.
Baseline functionality
The upstream library has five main methods:
In vk_safe_struct_utils.hpp:
safe_<type>()constructors (per type)AddToPnext(safe_base, addition)RemoveFromPnext(safe_base, removal)
In vk_struct_helper.hpp:
T* FindStructInPNextChain<T>(void* base_ptr)const T* FindStructInPNextChain<T>(const void* base_ptr)
Functional issues
The purpose of safe-struct is that it generates a modifiable deep-copy of the input structures from the user, allowing layers to mess around with "const" data. It does this by allocating a new safe_<type> structure per Vulkan structure in the tree and for every variable length array or string those structs reference.
The library uses non-const pointers for things that are pointers to other safe_structs, but uses const pointers for things that are pointers to primitive types if they were const in the original Vulkan structure we are impostering. We can legally const_cast these, because we know the memory is writable, but needing const_cast is a bit of a code smell.
One specific case of this const_cast issue is FindStructInPNextChain(), which we need to use to find a copy of a user structure in the pNext tree. The pNext pointer in each safe_<T> is const void*, so by default matches the const overload of the search function and returns a const T*. If you want to modify the structure the caller must either const_cast the parameter, or const_cast the result. In the general case of searching a user-owned pNext chain this is correctly restrictive, but in the case of a chain owned by a safe_<T> type the cast in the calling code is unnecessary.
Option 1: make the value types in the safe_<T> structs non-const, making them easier to fiddle with them.
Option 2: make pointer-types in the safe_<T> structs non-const, which makes them easier to fiddle with. However, layer code fiddling with these can leak memory if it's not careful if dropping a pointer to a safe_<T> without deleting it, so this may be better achieved via accessor methods.
Option 3: add a new FindInPnext<T>(safe_base) function for use when the base class is a safe-struct, which accepts a safe_<T> type as the base pointer, which gives us the guarantee that these are mutable and can hide the const-cast inside the library rather than the calling code.
More generally the current code violates strict-aliasing as it replaces one type with another, and then relies on reinterpret_cast<> to flip between them.
API cleanups
The Add/Remove functions currently try to ensure safety by asserting that safe_base.ptr() exists, but this is a somewhat imperfect check and will only trigger in debug builds with asserts enabled. All the safe_struct types should probably inherit from an abstract base and then we can leverage the compiler to filter the template on std::is_base_of.
AddToPnext(safe_base, addition)RemoveFromPnext(safe_base, removal)
Performance issues
The major design issue with safe-struct is that it makes a huge number of memory allocations, deep copying everything with a unique allocation per struct or array. This can results in dozens of memory new/delete cycles per Vulkan API call. For our current layers this is not really a problem because our need-to-be-fast performance measurement layers only edit user structures for instance/device creation which is not expected to be fast.
Idea 1: In the general case most of these copies are unnecessary - we often need a partial copy, not a full copy of the entire tree. What to copy will depend on what the layer needs to modify.
Idea 2: Memory allocation could be via a simple linear allocator which allocates large chunks of memory when needed, and then sub-allocates from them with simple linear increments for each copied struct or array it needs to allocate. All chunks are released when the function returns.
Idea 3: safe_<T> always allocates a copy so that the tree owns the memory, which is useful for the original user memory, but is unnecessary for later patches where the memory is already owned by the layer. Future API should avoid this.
Idea 4: safe_<T> often requires multiple allocations - an allocation for the original copy of the user data, and then a second allocation and another copy when a patch needs more space in an array. Possible to declare required size up front so we have spare space to patch into?