Release native vertex-array layout references - #1896
bkaradzic-microsoft wants to merge 3 commits into
Conversation
Port the focused shotgun vertex-layout ownership fix onto official master. Balance every acquired layout reference, reject duplicates before allocation, release on insertion failure, report exhaustion, and guard stale device generations. Add seven native lifecycle and exhaustion regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 723c9021-b79c-4089-9ff7-0d8ad63f1e98
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Fixes VertexArray lifetime management so bgfx::VertexLayoutHandle references are correctly released, including in failure paths and across device loss, and adds regression tests to prevent reintroducing leaks/exhaustion.
Changes:
- Make
VertexArraytrackDeviceContext+ device generation and release owned layout references on dispose/destruct (once). - Prevent duplicate attribute recording before allocation, clean up on insertion failure, and throw when layout allocation fails.
- Add unit tests validating layout accounting across lifecycle, sharing, duplicates, exhaustion/recovery, and device loss.
| File | Description |
|---|---|
| Plugins/NativeEngine/Source/VertexArray.h | Makes VertexArray device-context-aware by storing DeviceContext + device id. |
| Plugins/NativeEngine/Source/VertexArray.cpp | Implements layout-handle release on disposal, duplicate rejection, disposed-guard, and allocation failure handling. |
| Plugins/NativeEngine/Source/NativeEngine.cpp | Updates VertexArray construction to pass m_deviceContext. |
| Apps/UnitTests/Source/Tests.NativeEngine.VertexArray.cpp | Adds 7 regression tests for layout lifecycle/accounting and device loss. |
| Apps/UnitTests/CMakeLists.txt | Registers the new VertexArray test file in the unit test target. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Use public layout padding in the exhaustion regression, handle unsuccessful insertion through the existing reference cleanup path, and include allocation context in layout errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 723c9021-b79c-4089-9ff7-0d8ad63f1e98
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (3)
VertexArraynow capturesm_deviceIdandDispose()conditionally destroys layouts only when… · Newstatic_cast<void>(attrib);is only needed because the structured binding namesattribbut… · New
Resolved since last review (3)
| { | ||
| throw std::runtime_error{"Cannot record a vertex buffer in a disposed vertex array"}; | ||
| } | ||
|
|
There was a problem hiding this comment.
Fixed in 0dc7bf2. RecordVertexBuffer now rejects a device-generation mismatch before either the per-vertex allocation path or the instance-record path. The new OldDeviceArrayRejectsNewRecords regression uses an empty pre-reset array and a fresh post-reset buffer, so the duplicate-attribute check cannot mask the bug. It covers divisors 0 and 1 and checks that no instances or layout resources are retained. All eight lifecycle tests passed in three consecutive runs on macOS/JSC with the Noop renderer.
| const auto layoutHandle = bgfx::createVertexLayout(layout); | ||
| if (!bgfx::isValid(layoutHandle)) | ||
| { | ||
| throw std::runtime_error{"Multiple vertex buffers with the same attribute cannot be recorded"}; | ||
| throw std::runtime_error{"Failed to create vertex layout (attribute=" + std::to_string(location) + | ||
| ", stride=" + std::to_string(byteStride) + ", offset=" + std::to_string(byteOffset) + | ||
| "). The maxVertexLayouts limit of " + std::to_string(bgfx::getCaps()->limits.maxVertexLayouts) + " may be exhausted"}; | ||
| } | ||
| try | ||
| { | ||
| if (!m_vertexBufferRecords.try_emplace(attrib, vertexBuffer, byteOffset / byteStride, layoutHandle).second) | ||
| { | ||
| throw std::runtime_error{"Multiple vertex buffers with the same attribute cannot be recorded"}; | ||
| } | ||
| } | ||
| catch (...) | ||
| { | ||
| bgfx::destroy(layoutHandle); | ||
| throw; | ||
| } |
There was a problem hiding this comment.
Keeping the current explicit cleanup here. The pre-allocation find prevents acquiring a reference for normal duplicate calls; the insertion-result check is a defensive ownership check requested in the earlier review. Both a false insertion result and a throwing insertion use the same catch to destroy exactly one acquired reference and rethrow. A separate guard would not change that behavior, and dropping the result check would reverse the earlier requested protection. The duplicate, disposal, sharing, and exhaustion/recovery regressions continue to pass.
| for (const auto& [attrib, record] : m_vertexBufferRecords) | ||
| { | ||
| static_cast<void>(attrib); |
There was a problem hiding this comment.
Addressed in 0dc7bf2: disposal now iterates entries and accesses pair.second, matching the traversal style in SetVertexBuffers. The unused structured binding and suppression are gone.
Reject new per-vertex and instance records after device loss, cover the stale-array path without allocating resources, and avoid an unused binding in disposal. Include GoogleTest before platform headers to prevent the X11 Bool macro from breaking all five Linux CI builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 723c9021-b79c-4089-9ff7-0d8ad63f1e98
|
CI follow-up: all five failing Linux jobs in run 35795505850 failed compiling the new test at gtest-param-test.h:360 because X11 defines Update: the replacement CI run 35883901384 has completed successfully. All 35 PR checks now pass, including all five Linux jobs that previously failed. |
Summary
Extracts the focused vertex-layout lifetime repair from the BabylonNative shotgun worktree, independently based on official master
b8c93d24.Each
bgfx::createVertexLayoutcall acquires a reference, including when a matching layout already exists. VertexArray previously dropped its records without releasing these references, and duplicate attribute insertion could acquire another reference before throwing.Validation
Built this isolated branch with the official dependency pins, macOS/JavaScriptCore, RelWithDebInfo, and
BABYLON_NATIVE_TESTS_USE_NOOP_METAL_DEVICE=ON.UnitTests --gtest_filter="NativeEngineVertexArray.*": 7 passed. These exercise bgfx resource accounting with the Noop renderer; this is not a claim of a fresh GPU rendering sweep.No bgfx/JsRuntimeHost changes, dependency updates, visual references, tolerances, or exclusions are included.