Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces robust device-lost tracking, frame throttling for embedder-managed images, and memory capping to prevent host memory exhaustion and driver crashes in Impeller's Vulkan backend. It also adds workarounds for Mesa dzn drivers and refines Vulkan barriers and subpass dependencies. Feedback on these changes highlights several issues: a Vulkan destruction order violation when handling allocation failures in allocator_vk.cc, a potential barrier validation warning in blit_pass_vk.cc due to layout assumptions, an ineffective thread-local rate limit for process-wide working set trimming, and potential crashes during teardown or resize on lost devices if invalid image views and fences are destroyed instead of released.
|
Acknowledging the bot's review. I'm currently short on time, but I will go through this in detail later. At a quick glance: the bot caught some valid edge cases (like the teardown paths on a lost device and the I will sort through the valid points, implement the fixes locally, and push an update as soon as I have time to get back to this. |
e1ccdad to
867bb91
Compare
gaaclarke
left a comment
There was a problem hiding this comment.
OP said he want's to address feedback from the bot. I'll put this into "request changes" state to reflect that.
I've also skimmed through the PR and approved it for CI execution so we can make sure everything is good there.
…urce exhaustion Stability fixes for the Vulkan backend discovered through sustained stress testing on AMD RDNA 2 hardware and Mesa dzn (Vulkan on D3D12 in WSL2), first published as part of flutter#183382 and rebased onto the current FenceWaiterVK submission architecture. Device loss handling: - ContextVK gains an atomic device_lost_ flag. CreateCommandBuffer() and CommandQueueVK::Submit() short-circuit once the device is lost, since any Vulkan call on a driver that entered a corrupted OOM state can access-fault inside the ICD. - On vkQueueSubmit failure, tracked objects are abandoned without making Vulkan calls (AbandonForDriverCrash on command pools and descriptor pools), the device is marked lost, and per-heap memory budgets are logged for diagnosis. - FenceWaiterVK releases rather than destroys the fence when the submit callback fails; some drivers partially track the fence even though the submission failed, and vkDestroyFence then crashes (VUID-vkDestroyFence-fence-01120 observed on AMD). Submission backpressure: - CommandQueueVK caps concurrent in-flight submissions (3) with a condition variable, releasing the slot from the fence completion callback. Prevents unbounded host memory growth when the CPU outpaces the GPU. Only frame submissions through Submit() are gated; SubmitWithReceipt() bypasses the gate so image uploads from the IO thread stay non-blocking (flutter#190445). - ContextVK::FlushCommandBuffers() submits in chunks of 64 to keep per-submit driver allocations bounded on frames that accumulate very large command buffer counts. - GPUSurfaceVulkanImpeller (embedder-managed image path) adds a two-entry frame fence ring mirroring the KHR swapchain's backpressure, with a bounded wait that skips the frame under extreme GPU pressure. Image views for embedder images are now cached instead of being allocated (and leaked) per frame, and in-flight frames are drained before views are destroyed on resize or teardown. Command pool lifecycle: - BackgroundCommandPoolVK releases its handles without Vulkan calls when the recycler is gone; the device may already be destroyed at that point. This is the same class of teardown race as the CommandPoolVK destructor fix that landed in flutter#186749. - The recycled pool list is capped at 8 entries; once the cache is full, further reclaimed pools are dropped, and RAII destroys their recycled buffers before the pool. Evicting from the vector instead would move-assign RecycledData in declaration order, destroying the overwritten entry's pool before freeing its buffers - a use-after- free inside the driver that corrupted the heap under SwiftShader in the canvas_test dart suite. A regression test asserts the free- before-destroy ordering on the drop path. - unused_command_buffers_ is annotated IPLR_GUARDED_BY(pool_mutex_). Descriptor pools: - Reset via vkResetDescriptorPool on reclaim. Reclaimed pools previously stayed at capacity and produced VK_ERROR_OUT_OF_POOL_MEMORY on every reuse. Barrier and render pass correctness: - Six barrier fixes in the blit pass: pre-copy barriers use transfer stage and access flags, post-copy barriers flush transfer writes, and mipmap generation barriers match the actual current layout. Fixes corrupted glyph atlases and SYNC-HAZARD-WRITE-AFTER-WRITE reported by the synchronization validation layer on AMD RDNA 2. - RenderPassBuilderVK moves subpass dependencies to a counted builder, drops eByRegion from external (VK_SUBPASS_EXTERNAL) dependencies where it has no meaning per Vulkan spec section 7.1, and gains SetFramebufferFetchEnabled() so input attachment references and self-dependencies are omitted on drivers without framebuffer fetch support. The flag is wired through pipeline and render pass creation and the compat render pass used for PSO construction. Driver workarounds: - DriverInfoVK exposes GetDriverID() (Vulkan 1.2) and detects Mesa dzn. - New skip_sub_region_buffer_to_image_copy workaround: Mesa dzn reports minImageTransferGranularity of (0,0,0) and corrupts textures on sub-region buffer-to-image copies; the blit pass stages such copies through a full-size temporary texture. Memory management: - The VMA buffer pool is capped at 256 blocks (about 1 GB); the pool otherwise grows without bound when high-water-mark demand spikes and fragmentation prevents block reuse. - Image allocation failure paths no longer leak the VMA allocation when image view creation fails. - On Windows, the working set is periodically trimmed when Resizable BAR mappings inflate RSS far beyond actual VMA usage (gated by cooldown, RSS threshold, and RSS-to-VMA ratio). KHR swapchain robustness: - The synchronizer fence is re-signaled with an empty submit on every early return after a failed or unusable acquireNextImageKHR (including the surface-lost path added in flutter#183338). The fence was reset by WaitForFence, and without a new signal the next wrap-around to the same synchronizer deadlocks in waitForFences with an infinite timeout. - AcquireNextDrawable short-circuits when the device is marked lost. - Zero-extent surfaces (minimized windows) skip the frame instead of attempting swapchain recreation, preserving recovery on restore. Embedder mode: - CapabilitiesVK enumerates instance layers in embedder mode so that validation layers can be detected and enabled when requested.
…ottle tests Adjust the descriptor pool changes to keep the current set-recycling semantics (used sets return to the unused cache on reclaim) instead of resetting pools on reclaim, which would have invalidated the cached set handles that DescriptorsAreRecycled encodes as intended behavior. The safety fixes remain: AbandonForDriverCrash, handle release on teardown when the context or recycler is gone, propagating CreateNewPool failures, and registering a new descriptor set in the cache only after the allocation succeeded (a garbage handle was previously cached when vkAllocateDescriptorSets failed). Guard the DeathRattle test helper against invoking a moved-from callback. WaitForReclaim moves the local instance into a UniqueResourceVKT, and the moved-from local still runs the destructor; calling the empty std::function aborts with bad_function_call. This reproduces on unmodified master when running impeller_unittests on a Windows host and made every CommandPoolRecyclerVKTest case crash there. New tests: - CommandQueueVKTest.SubmitAfterDeviceLostIsCancelled: no submission reaches the queue once the device is marked lost. - CommandQueueVKTest.ThrottleAllowsSustainedSubmissions: in-flight slots are released as submissions complete. - ContextVKTest.CreateCommandBufferShortCircuitsAfterDeviceLost.
An embedder that presents rendered images itself, for example through a platform compositor, supplies a Vulkan device without the surface, WSI, or swapchain extensions. CapabilitiesVK previously rejected such a device because it required VK_KHR_surface and a windowing-system integration extension. Relax those requirements in embedder mode when the surface extension is absent, so presentation becomes the embedder's responsibility. Embedders that do provide the surface extension are unaffected. Adds regression tests covering the surfaceless case and the case where the embedder still provides the surface extension.
Follow-ups to the first review pass on the Vulkan hardening change: - AllocatedTextureSourceVK: destroy the already-created image views before the image when a render target view fails to create partway through the per-subresource view loop. - BlitPassVK: pick the destination barrier's source scope from the texture's tracked layout in OnCopyTextureToTextureCommand, mirroring OnCopyBufferToTextureCommand. The unconditional eTransferWrite scope can trip best-practices validation when the destination was last sampled. The atlas growth WAW hazard stays covered: that copy takes the eFragmentShader arm, which chains with the eTransfer -> eFragmentShader barrier from the preceding buffer upload; the old eTopOfPipe scope could not form that chain. - AllocatorVK: make the Windows working-set trim cooldown a process-wide atomic instead of thread_local, since EmptyWorkingSet affects the whole process. - GPUSurfaceVulkanImpeller: on a lost device, abandon the cached image views and frame fences instead of waiting on or destroying them, in both the destructor and the resize drain, matching the AbandonForDriverCrash policy. The teardown-after-device-loss test captures the raw handles and destroys them itself after teardown, which verifies the abandonment (a destroy inside the surface would turn these into double destroys) and keeps the test leak-free under LeakSanitizer. - GPUSurfaceVulkanImpeller: the teardown drain only runs when an embedder delegate is present. Without one (flutter_tester) there are no fences or cached views to drain, and the context is a SurfaceContextVK, so casting it to ContextVK was invalid.
867bb91 to
dad5498
Compare
|
All bot feedback is addressed, replies are in the threads. Additionally:
Verified on Windows ( |
gaaclarke
left a comment
There was a problem hiding this comment.
Hi @sero583 thanks for the PR. There is some really promising work in here. I'm going to have to unfortunately ask you to break it up into smaller PRs though. Most of these changes are dealing with really fiddly parts of the engine. We want to be able to bisect them individually and more clearly see how they are tested.
At first glance it seems there is potentially a lot of new logic that isn't being tested. We want to make sure these fiddly parts don't get accidentally broken in the future.
| << vk::to_string(result); | ||
| // Nothing owns the image yet; destroy it to avoid leaking the | ||
| // allocation. | ||
| vmaDestroyImage(allocator, vk_image, allocation); |
There was a problem hiding this comment.
use std::unique_ptr with a custom deleter instead of inserting all these cleanup calls.
| // they are faulted back in on next access, so the cost is minimal for | ||
| // pages that are actively used. | ||
| { | ||
| static constexpr size_t kTrimCooldownFrames = 300; // ~5 s at 60 fps. |
There was a problem hiding this comment.
Can we choose 5s instead of assuming fps will be 60?
| trim_cooldown.store(cooldown - 1, std::memory_order_relaxed); | ||
| } | ||
|
|
||
| if (cooldown == 0) { |
There was a problem hiding this comment.
since it's an int
| if (cooldown == 0) { | |
| if (cooldown <= 0) { |
|
Hey @gaaclarke, thanks for the review, and no argument on the split. It grew while I was chasing crashes on AMD and Mesa dzn and I kept adding to the same branch. I will break it up by subsystem, one PR per area, each with its own tests: device loss handling, command pool lifecycle, descriptor pool recycling, blit pass barriers, render pass subpass dependencies, submission backpressure and the frame fence ring, memory management, driver workarounds, KHR swapchain robustness, embedder mode capabilities. I will start on it in the next few days. If you would like them in a particular order, or would rather see some of those folded together, please let me know and I will follow that. Otherwise I will start with the self contained ones. Your three inline comments I will carry into the relevant PR rather than patching this one, since it is going away. I am moving this one to draft in the meantime so it does not sit in the review queue while I split it, and I will close it once the split PRs are up and the approach looks right to you. It stays as the reference in case you want to look something up in context. One thing worth flagging: the command pool cache eviction had a use after free, the vector erase move-assigns the entries and destroys a pool before its command buffers are freed. I reproduced it deterministically against SwiftShader and it is my best explanation for the Thanks again for taking the time on this. |
This PR hardens Impeller's Vulkan backend against driver failures and resource exhaustion. Every fix traces to a specific crash, validation error, or unbounded-growth pattern observed during sustained stress testing on AMD RDNA 2 hardware (Radeon RX 6750 XT, Windows 10) and Mesa dzn (Vulkan on D3D12 under WSL2), using a 9-scene GPU benchmark suite (https://github.com/sero583/flutter-benchmark).
This is the first PR of the series splitting #183382 (Impeller Vulkan for Linux and Windows desktops), per review guidance there; design doc: https://flutter.dev/go/impeller-backend-desktop. The changes here are platform-agnostic and benefit the existing Android Vulkan path directly. Several issues addressed by the original branch were independently reported and fixed after that work was first published in March (#185122, #186749, #187761); this PR rebases onto those and ports the remaining fixes.
Device loss handling
On some drivers (observed on AMD), a
vkQueueSubmitthat fails withVK_ERROR_OUT_OF_HOST_MEMORYleaves the driver in a corrupted state: the command pool and command buffer handles passed to the failed submission are freed internally, and any further Vulkan call (includingvkDeviceWaitIdle) can access-fault inside the ICD.ContextVKgains an atomicdevice_lost_flag withIsDeviceLost()/MarkDeviceLost().CreateCommandBuffer()andCommandQueueVK::Submit()short-circuit once lost, andKHRSwapchainImplVK::AcquireNextDrawable()skips the frame.AbandonForDriverCrash()on command pools and descriptor pools), the device is marked lost, and per-heap usage/budget is logged (VK_EXT_memory_budgetwhen the driver fills the chained struct) to make field reports diagnosable.FenceWaiterVKreleases rather than destroys the fence when the submit callback fails. Some drivers partially track the fence even though the submission failed; destroying it triggersVUID-vkDestroyFence-fence-01120and crashes. This extends the reasoning of [Impeller] Move queue submission into a callback that is invoked by FenceWaiterVK::AddFence only if it can accept the fence #187761 to the submit-failure path.Submission backpressure
Without backpressure, the CPU queues submissions faster than the GPU drains them; fences, tracked resources, and driver objects accumulate until
VK_ERROR_OUT_OF_HOST_MEMORY(reproducible within minutes at uncapped frame rates in debug builds).CommandQueueVKcaps concurrent in-flight submissions (3 =kMaxFramesInFlight+ 1) with a condition variable; the slot is released from the fence completion callback. A generous 5 s timeout cancels the submission rather than queueing unbounded work. TheGpuSubmissionTrackerbookkeeping introduced by [Impeller] Recycle HostBuffer arena entries only after GPU completion #188965 is preserved on every path: completions are recorded exactly once whether the submission succeeds, fails invkQueueSubmit, or is rejected by the fence waiter, so HostBuffer arena recycling never stalls.ContextVK::FlushCommandBuffers()submits in chunks of 64 so per-submit driver allocations stay bounded on frames that accumulate very large command buffer counts (heavy blur/filter workloads).GPUSurfaceVulkanImpellergains a two-entry frame fence ring mirroring the KHR swapchain'sWaitForFencebackpressure, with a bounded wait that drops the frame under extreme pressure. Image views for embedder-provided images are now cached instead of allocated (and leaked) every frame, and in-flight frames are drained before views are destroyed on resize or teardown (VUID-vkDestroyImageView-imageView-01026).Command pool lifecycle
BackgroundCommandPoolVKreleases its handles without Vulkan calls when the recycler is gone; the device may already be destroyed at that point. Same class of teardown race as [Impeller][Vulkan] CommandPoolVK can free command buffers on a destroyed VkDevice, crashing in vkFreeCommandBuffers on the resource manager thread #186458, on the path Saves a DeviceHolderVK with the CommandPoolVK #186749 did not cover.unused_command_buffers_is annotatedIPLR_GUARDED_BY(pool_mutex_), matchingcollected_buffers_.Descriptor pools
AbandonForDriverCrash()releases pool handles without Vulkan calls after a fatal submission failure, and the destructor releases handles when the context or recycler is already gone during teardown.vkAllocateDescriptorSetsfailed.CreateNewPoolfailures during the out-of-pool-memory fallback are propagated instead of retrying with an empty pool list.Barrier and render pass correctness
BlitPassVK: pre-copy barriers use transfer stage/access, post-copy barriers flush transfer writes, and mipmap barriers match the actual current layout. Fixes corrupted glyph atlases andSYNC-HAZARD-WRITE-AFTER-WRITEreported by the synchronization validation layer on AMD RDNA 2.RenderPassBuilderVKmoves subpass dependencies to a counted builder, removeseByRegionfromVK_SUBPASS_EXTERNALdependencies (meaningless per Vulkan spec section 7.1 outside self-dependencies), and gainsSetFramebufferFetchEnabled()so input attachment references and self-dependencies are omitted on drivers without framebuffer fetch support (Mesa dzn rejects them). The flag is wired through pipeline creation, render pass creation, and the compat render pass used for PSO construction.Driver workarounds
DriverInfoVKexposesGetDriverID()(Vulkan 1.2) and detects Mesa dzn.skip_sub_region_buffer_to_image_copyworkaround: Mesa dzn reportsminImageTransferGranularityof (0,0,0) and corrupts textures on sub-region buffer-to-image copies;BlitPassVKstages such copies through a full-size temporary texture.Memory management
EmptyWorkingSet, gated by cooldown, RSS threshold, and RSS-to-VMA ratio) when Resizable BAR VRAM mappings inflate RSS to several GB while actual VMA usage is small. This is cosmetic (the pages are GPU-mapped VRAM, not leaks) but prevents alarming numbers in monitoring tools. Happy to split this into a follow-up if reviewers prefer platform-specific code out of allocator_vk.cc.KHR swapchain robustness
acquireNextImageKHR, including theeErrorSurfaceLostKHRpath added by [Impeller] Do not log VK_ERROR_SURFACE_LOST_KHR errors returned by vkAcquireNextImageKHR #183338.WaitForFenceresets the fence before the acquire; without a new signal, the next wrap-around to the same synchronizer deadlocks inwaitForFenceswith an infinite timeout.Embedder-controlled presentation
CapabilitiesVKallows an embedder that presents rendered images itself (for example through a platform compositor) to create a Vulkan context without the surface, WSI, or swapchain extensions. Previously a device withoutVK_KHR_surfacewas rejected outright. The relaxation only activates in embedder mode when the surface extension is absent; embedders that do provide it are unaffected. This is the enabling change for the surfaceless Windows DirectComposition backend, and is a no-op for existing (Android, KHR-swapchain) users.Testing
New unit tests:
CommandQueueVKTest.SubmitAfterDeviceLostIsCancelled: no submission reaches the queue once the device is marked lost.CommandQueueVKTest.ThrottleAllowsSustainedSubmissions: in-flight slots are released as submissions complete (more submissions than the cap).ContextVKTest.CreateCommandBufferShortCircuitsAfterDeviceLost.ContextVKTest.EmbedderWithoutSurfaceExtensionsIsSurfaceless: an embedder device with no surface/swapchain extensions creates a valid context.ContextVKTest.EmbedderWithSurfaceExtensionsStillEnablesThem: when the embedder provides the surface extension it is still honored.Test infrastructure fix: the
DeathRattlehelper incommand_pool_vk_unittests.ccinvoked a moved-fromstd::functionfrom its destructor (WaitForReclaimmoves the local into aUniqueResourceVKT), which aborts withbad_function_callwhen runningimpeller_unittestson a Windows host and took the entireCommandPoolRecyclerVKTestsuite down with it. With the guard in place, that suite andCommandPoolVKTest.DestroysCleanlyIfDeviceIsDestroyed(previously also crashing on Windows hosts) run and pass on this branch.Suite results on a Windows host with this branch rebased onto current master: the full
impeller_unittestssuite runs 4550 tests, 2960 passed, 0 failed (the remainder are playground variants skipped as unsupported on the host). Two pre-existing issues are excluded as unrelated: the color emoji playground crashes (#189565, fix in #189572) and a debug-STL assert in a display_list gradient-stops test that reproduces identically on unmodified master.Not unit-testable with the current mock (noted for reviewers): the fence release on failed submission (the mock's
vkQueueSubmitcannot be made to fail) and Mesa dzn detection viadriverID(the mock exposes noVkPhysicalDeviceVulkan12Properties). Extending the mock for failure injection could be follow-up work.Manual: 9-scene benchmark suite on AMD RDNA 2 (Windows 10) and Mesa dzn (WSL2), no crashes, deadlocks, or validation errors under VK_LAYER_KHRONOS_validation with sync validation enabled.
Part of #181711
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.