Repository navigation
layersvt: Add common layer foundation library and unit tests - #31
Conversation
7a7b20b to
d175583
Compare
emrekultursay
left a comment
There was a problem hiding this comment.
I did a first round of review in good faith, but it took me much more time than I'd like to reserve for such a change.
I'd like to give a gentle warning that such changes put a high code review burden on the reviewer to verify that this is correct or makes the right set of changes. AI code review can help find interesting corner cases, but at the end of the day, I read "every single line", and ask "why was this done?" and "is this the best way?" for each of them, and there's hundreds of such questions in this PR.
For instance, when I look at this PR, the first thing that hits me is that there's lots of dead code (in fact the entire PR is dead code, it's not used by any layer). Then I look at follow-up PR which uses this in DebugMarker layer, and I notice that some methods (e.g., DispatchDownstream) I thought were dead are used there, so it's not dead, but there's still some other code (e.g., log.h) that's still dead. But will a third PR use it? That's just one small aspect.
Then there's also some questionable stuff (e.g., keep_alive change). Those deserve a conversation of their own; but get lost in such a big PR that changes too many things in one shot.
To make it more reviewer friendly (and get changes reviewed faster with less complaints), I'd recommend splitting them differently, e.g., here I'd split on a feature-by-feature basis, like this:
PR 1. Extract and empty base class. Move all layers to that base class. No-op change, takes 1 minute to review.
PR 2. Add manifest to base class constructor, move all layers to use the base class constructor.
PR 3. Add DeviceInstanceTracker to base class, move all layers to use it.
PR 4. Add EagerMapDevices, move all layers to use it. If any layer doesn't need it, add a comment instead of a call to EagerMapDevices.
PR 5. Move the keep_layer_alive functionality. Get precise code review feedaback, explore STATIC vs OBJECT.
PR 6. ...
Every single PR introduces one small functionality, comes with the unit tests for that functionality, does not introduce unused/dead code or tests.
I am guessing that's how you implemented and verified this yourself: step-by-step. Then, as the reviewer, if I see the progression in the same way, it'd be much easier for me to understand their impact, and verify.
To make this PR (and the follow-up) move forward with least effort, one option is to keep this PR, but split it into various commits within the PR, where each one does that one functionality. Then I (and you and AI) can review and verify each commit separately. |
940deba to
0d0ec15
Compare
|
The PR has been reorganized into 10 cohesive, atomic commits with co-located unit tests in
All review comments have been addressed in their respective commits, and all presubmit checks are passing. |
|
You can start from high level review of the common layer library documentation in README.md |
Introduce DispatchTableManager to provide thread-safe storage, initialization, and lookup for Vulkan instance and device dispatch tables. Key components: - Thread-safe storage with std::mutex - GetDispatchKey() helper for dispatchable handles - Loader callback tracking (VK_LOADER_DATA_CALLBACK) Bug: Test: new tests - DispatchTableManagerTest#GetDispatchKey, DispatchTableManagerTest#LoaderDataCallback, DispatchTableManagerTest#InstanceAndDeviceTableLifecycle, DispatchTableManagerTest#ConcurrentAccess Change-Id: I73cf682789015e4d41ab6f42f03964f16a6a6964
98224a3 to
2e27ce4
Compare
2e27ce4 to
277232f
Compare
…ager Resolve VkPhysicalDevice handles to their parent VkInstance through the shared dispatch key (loader ABI), without per-physical-device tracking: - Record the owning VkInstance per instance dispatch key and add GetVkInstance(VkPhysicalDevice). - Add strongly-typed GetInstanceDispatchTable(VkPhysicalDevice) and GetInstanceDispatchTable(std::nullptr_t) overloads. - Clear the instance key mapping in DestroyInstanceTable. - Guard GetDeviceDispatchTable against null object handles. Bug: Test: new tests - DispatchTableManagerTest#PhysicalDeviceResolvesInstanceViaDispatchKey, DispatchTableManagerTest#PhysicalDeviceUnresolvedAfterInstanceDestroy, DispatchTableManagerTest#NullHandleSafety Change-Id: I5b05dc0b42701d659485c3966c4ca6be6a6a6964
Link layer_keep_alive.cpp into layersvt_common OBJECT library on Android: - Move the existing layer_keep_alive.cpp unchanged into layersvt/common; its constructor calls dlopen(..., RTLD_NODELETE) to keep the layer shared object resident in process memory across Vulkan loader queries. - Linking as part of CMake OBJECT library ensures the constructor is preserved by the static linker without requiring explicit header declarations or runtime call sites. Bug: Test: n/a Change-Id: I62267c72611916d0c28fdbec203d27b96a6a6964
…ting Introduce DispatchDownstream template helpers in dispatch_downstream.h and establish LayerBase command intercept routing and singleton tracking: - Implement DispatchDownstream, DispatchDownstreamOr, DispatchDownstreamOrSuccess, and DispatchDownstreamIfAvailable to enable type-safe forwarding of Vulkan commands to downstream dispatch tables. - Establish LayerBase singleton lifecycle (Get()) and accessor to DispatchTableManager. - Implement GetInstanceProcAddr and GetDeviceProcAddr with GetKnownInstanceCommand and GetKnownDeviceCommand dispatch tables, supporting virtual layer hook overrides (GetLayerInstanceCommand and GetLayerDeviceCommand) that receive the queried instance or device handle. Bug: Test: new tests - DispatchDownstreamTest#DispatchDownstream, DispatchDownstreamTest#DispatchDownstreamIfAvailable, LayerBaseTest#LayerTracking, LayerBaseTest#GetKnownCommandsCommonWithoutManifest, LayerBaseTest#LayerSpecificOverrideHooks, LayerBaseTest#ProcAddrDispatchChain Change-Id: I47a06c52a0a410f498c86084855369896388f87d
Implement Vulkan loader chain traversal and Template Method lifecycle hooks for instance and device creation and teardown: - Implement GetChainInfo helper to inspect and unwrap VkLayerInstanceCreateInfo (VK_LAYER_LINK_INFO) and VkLayerDeviceCreateInfo (VK_LAYER_LINK_INFO and VK_LOADER_DATA_CALLBACK). - Add extensible pre/post virtual lifecycle hooks (PreCreateInstance, PostCreateInstance, PreDestroyInstance, PreCreateDevice, PostCreateDevice, PreDestroyDevice). - Implement CreateInstance, DestroyInstance, CreateDevice, and DestroyDevice intercepts managing dispatch table initialization, loader callback registration, and teardown ordering. Bug: Test: new tests - LayerBaseTest#HookInvocations, LayerBaseTest#CreateInstanceWithMockChain, LayerBaseTest#CreateInstanceNullHandling, LayerBaseTest#PreCreateNotInvokedOnMissingChain, LayerBaseTest#CreateInstanceNullSafetyInHook, LayerBaseTest#PreCreateInstanceMutation, LayerBaseTest#PreCreateDeviceMutation, LayerBaseTest#TeardownOrdering, LayerBaseTest#CreateDeviceWithMockChain, LayerBaseTest#CreateDeviceNullHandling, LayerBaseTest#CreateDeviceInvalidInputSafety, LayerBaseTest#CreateInstanceNullFpCreateInstance, LayerBaseTest#CreateDeviceNullFpCreateDevice, LayerBaseTest#DefaultHooksExecution, LayerBaseTest#DestroyNullHandles, LayerBaseTest#LayerCommandHooksReceiveHandles Change-Id: Iefeb5682a4c19b9da9073e17145d0f506a6a6964
Cover LayerBase physical device behavior that relies on the DispatchTableManager dispatch-key resolution: - Verify a physical device resolves its parent instance and instance dispatch table through LayerBase. - Verify vkEnumeratePhysicalDevices and vkEnumeratePhysicalDeviceGroups are not intercepted and route to the downstream implementation. - Add layer_test::ResetLayer coverage for repeated and cross-type resets. Bug: Test: new tests - LayerBaseTest#PhysicalDeviceResolvesInstanceTable, LayerBaseTest#EnumeratePhysicalDevicesRouteDownstream, LayerBaseTest#ResetLayerMultipleInvocations, LayerBaseTest#ResetLayerCrossType Change-Id: I4e94b8e21051b7f08b3e8c2536ca20956a6a6964
Introduce LayerManifest aggregate struct and integrate layer and extension property enumeration into LayerBase. Key capabilities: - Static layer and extension property definitions via LayerManifest - Downstream extension query and merging in LayerBase - Virtual instance and device extension filtering/augmentation hooks Bug: Test: new tests - LayerManifestTest, LayerBaseEnumerationTest, LayerBaseHooksTest, LayerBaseTest#GetKnownCommandsCommonWithManifest Change-Id: I96c027dad43270617323b2a61fe206196a6a6964
Integrate physical device tooling property enumeration and customization hooks into LayerBase: - Implement GetPhysicalDeviceToolProperties and downstream merging with Vulkan 1.3 and VK_EXT_tooling_info support. - Add HasToolProperties and ProcessToolProperties virtual hooks to customize or filter reported tool properties. - Conditionally intercept vkGetPhysicalDeviceToolProperties[EXT] in GetKnownInstanceCommand when HasToolProperties returns true. Bug: Test: new tests - LayerBaseEnumerationTest#ToolPropertiesMerge, LayerBaseEnumerationTest#ToolPropertiesErrorPropagation, LayerBaseHooksTest#ProcessToolPropertiesCustomizing, LayerBaseTest#GetKnownCommandsWithToolProperties, LayerBaseTest#GetPhysicalDeviceToolPropertiesDispatch Change-Id: I3a8b54c2910e47f18b2d69012c4e8f756a6a6964
277232f to
213fa8a
Compare
Implement layersvt_entrypoints OBJECT library for shared layer export: - Export standard C ABI entry points (vkGetInstanceProcAddr, vkGetDeviceProcAddr, and enumeration functions) delegating directly to LayerBase static methods. - Package as an OBJECT library target in CMake so Vulkan layer shared libraries can include $<TARGET_OBJECTS:layersvt_entrypoints> without duplicate symbol conflicts in test executables. - Link tool layers with -Wl,-Bsymbolic-functions on Linux so the exported entry points bind to the layer's own definitions when the layer is loaded into a process that also links libvulkan. Bug: Test: new tests - LayerEntrypointsTest#ForwardingCalls Change-Id: I0081acc9edf66f54efafe68639637ac66a6a6964
Document layersvt_common foundation library and new layer authoring: - Add comprehensive guide in layersvt/common/README.md covering LayerBase subclassing, declarative LayerManifest configuration, downstream command interception via DispatchDownstream, CMake integration via layersvt_entrypoints, and unit testing conventions. - Add LayerBase class contract documentation in layersvt/common/layer_base.h referencing layersvt/common/README.md. - Update layersvt/README.md to reference layersvt_common documentation. Bug: Test: n/a Change-Id: Ic951a67b73e3321a7cbe8be4d9fa41c46a6a6964
213fa8a to
8009a75
Compare
|
Restructured the PR into 10 atomic per-feature commits as suggested in your follow-up comment, with each commit building and passing
Also removed |
Implement the common layer support library (
layersvt_common) as an OBJECT library to provide shared, thread-safe infrastructure across Vulkan layers:VkPhysicalDevicetoVkInstancemapping usingstd::shared_mutexwith eager device enumeration (EagerMapDevices).DispatchDownstreamcompile-time dispatch helper and loader callback tracking.RTLD_NODELETEself-pinning with explicitEnsureLayerKeepAlive()to prevent static linker dead-stripping.VT_LOGI,VT_LOGW,VT_LOGElogging macros.test_common_layer): unit tests covering all common components, eager mapping, and lifecycle execution under a mock loader.Bug:
Test: new tests