layersvt: Add common layer foundation library and unit tests by olehkuznetsov · Pull Request #31 · android-graphics/VulkanTools

olehkuznetsov · 2026-09-02T18:55:18Z

Implement the common layer support library (layersvt_common) as an OBJECT library to provide shared, thread-safe infrastructure across Vulkan layers:

DeviceInstanceTracker: thread-safe VkPhysicalDevice to VkInstance mapping using std::shared_mutex with eager device enumeration (EagerMapDevices).
DispatchTableManager: thread-safe instance and device dispatch table storage with DispatchDownstream compile-time dispatch helper and loader callback tracking.
LayerManifest: declarative layer metadata container with downstream extension merging and Android b/143293104 compatibility workaround.
LayerBase: standardized layer lifecycle implementation with runtime loader chain validation, teardown ordering, and extensible virtual hooks for custom command interception.
layer_keep_alive: Android RTLD_NODELETE self-pinning with explicit EnsureLayerKeepAlive() to prevent static linker dead-stripping.
log.h: cross-platform VT_LOGI, VT_LOGW, VT_LOGE logging macros.
Unit test suite (test_common_layer): unit tests covering all common components, eager mapping, and lifecycle execution under a mock loader.

Bug:
Test: new tests

Implement the common layer support library (layersvt_common) as an OBJECT library to provide shared, thread-safe infrastructure across Vulkan layers: - DeviceInstanceTracker: thread-safe VkPhysicalDevice to VkInstance mapping using std::shared_mutex with eager device enumeration (EagerMapDevices). - DispatchTableManager: thread-safe instance and device dispatch table storage with DispatchDownstream compile-time dispatch helper and loader callback tracking. - LayerManifest: declarative layer metadata container with downstream extension merging and Android b/143293104 compatibility workaround. - LayerBase: standardized layer lifecycle implementation with runtime loader chain validation, teardown ordering, and extensible virtual hooks for custom command interception. - layer_keep_alive: Android RTLD_NODELETE self-pinning with explicit EnsureLayerKeepAlive() to prevent static linker dead-stripping. - log.h: cross-platform VT_LOGI, VT_LOGW, VT_LOGE logging macros. - Unit test suite (test_common_layer) covering all common components, eager mapping, and lifecycle execution under a mock loader. Bug: Test: new tests Change-Id: Id6bb4faeb1e545003242b61cbdf491356a6a6964

emrekultursay

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.

emrekultursay · 2026-09-03T18:07:00Z

+
+namespace {
+__attribute__((constructor)) void keep_alive_ctor() {
+    EnsureLayerKeepAlive();


layersvt_common is an OBJECT library (a bunch of .o files), not a STATIC library (.a). AFAIU, this means it never strips. So this trick to avoid stripping is redundant. You can delete the header, delete the redundant usage from base, and revert this file to the original form.

emrekultursay · 2026-09-03T18:10:46Z

 endif()

+if (NOT MSVC)
+    set_source_files_properties(../perfetto/perfetto.cc PROPERTIES COMPILE_OPTIONS "-Wno-deprecated-declarations")


What's this needed for? Is this related to this change?

emrekultursay · 2026-09-03T18:14:42Z

+
+    uint32_t count = 0;
+    VkResult result = enumerate_physical_devices(instance, &count, nullptr);
+    if ((result == VK_SUCCESS || result == VK_INCOMPLETE) && count > 0) {


When pPhysicalDevices is nullptr, it shouldn't return VK_INCOMPLETE. The test that introduced that non-standard behavior in the mock should also be deleted.

emrekultursay · 2026-09-03T18:17:15Z

+                    if (!property_count) return VK_ERROR_INITIALIZATION_FAILED;
+                    if (!physical_devices) {
+                        *property_count = 1;
+                        return VK_INCOMPLETE;


This shouldn't return VK_INCOMPLETE. when physical_devices is nullptr, it shouldn't return incomplete.

emrekultursay · 2026-09-03T18:22:26Z

+    DeviceInstanceTracker(const DeviceInstanceTracker&) = delete;
+    DeviceInstanceTracker& operator=(const DeviceInstanceTracker&) = delete;
+
+    mutable std::shared_mutex mutex_;


Using std::shared_mutex seems over-engineering. It may even make it worse because it's slower than std::mutex.

emrekultursay · 2026-09-03T18:46:53Z

+    }
+    if constexpr (!std::is_void_v<ReturnType>) {
+        if constexpr (std::is_same_v<ReturnType, VkResult>) {
+            return VK_SUCCESS;


Is this correct? Either table or MemberPtr is null. Should it be returning VK_SUCCESS?

emrekultursay · 2026-09-03T18:48:45Z

+        if constexpr (std::is_same_v<ReturnType, VkResult>) {
+            return VK_SUCCESS;
+        } else {
+            return ReturnType{};


Is this dead code in real life? Does anything return other than void/VkResult? If no, then consider replacing if/else with a static assertion.

static_assert(std::is_same_v<ReturnType, VkResult>, "DispatchDownstream fallback only supports void or VkResult functions.");

emrekultursay · 2026-09-03T18:54:37Z

@@ -0,0 +1,28 @@
+/* Copyright (C) 2026 Google Inc.


Even after the debug_marker PR, this is still not used.

emrekultursay · 2026-09-03T19:27:59Z

+target_link_libraries(layersvt_common PUBLIC
+    Vulkan::Headers
+    Vulkan::UtilityHeaders
+    Vulkan::LayerSettings


is this needed?

emrekultursay · 2026-09-03T19:31:47Z

+)
+
+target_include_directories(layersvt_common PUBLIC
+    ${CMAKE_CURRENT_SOURCE_DIR}


Do we need this line? Seems redundant.

emrekultursay · 2026-09-03T19:50:17Z

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.

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.

olehkuznetsov mentioned this pull request Sep 2, 2026

layersvt: Refactor DebugMarker to inherit from LayerBase #32

Open

olehkuznetsov requested a review from emrekultursay September 2, 2026 19:17

olehkuznetsov force-pushed the pr-layersvt-common branch from 7a7b20b to d175583 Compare September 2, 2026 19:52

emrekultursay reviewed Sep 3, 2026

View reviewed changes

Navigation Menu

Uh oh!

layersvt: Add common layer foundation library and unit tests - #31

layersvt: Add common layer foundation library and unit tests#31
olehkuznetsov wants to merge 1 commit into
android-graphics:mainfrom
olehkuznetsov:pr-layersvt-common

olehkuznetsov commented Sep 2, 2026

Uh oh!

emrekultursay left a comment

Uh oh!

emrekultursay Sep 3, 2026

Uh oh!

emrekultursay Sep 3, 2026

Uh oh!

emrekultursay Sep 3, 2026

Uh oh!

emrekultursay Sep 3, 2026

Uh oh!

emrekultursay Sep 3, 2026

Uh oh!

emrekultursay Sep 3, 2026

Uh oh!

emrekultursay Sep 3, 2026

Uh oh!

emrekultursay Sep 3, 2026

Uh oh!

emrekultursay Sep 3, 2026

Uh oh!

emrekultursay Sep 3, 2026

Uh oh!

emrekultursay commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

olehkuznetsov commented Sep 2, 2026

Uh oh!

emrekultursay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emrekultursay commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants