Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 5eb21d2

Browse files
author
Jonah Williams
authored
[Impeller] Reland: one descriptor pool per frame. (#55960)
reverted #55939 due to postsubmit failures. It turns out the lifecycle is not exactly the same as the command pool. Instead, use a thread id map on the context to keep the right thread running and simplify shutdown. Leaving CI yaml edit for now to prove it works :) Fixes flutter/flutter#157115
1 parent a489914 commit 5eb21d2

File tree

9 files changed

+72
-18
lines changed

9 files changed

+72
-18
lines changed

impeller/renderer/backend/vulkan/command_buffer_vk.cc

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,10 @@ namespace impeller {
2222
CommandBufferVK::CommandBufferVK(
2323
std::weak_ptr<const Context> context,
2424
std::weak_ptr<const DeviceHolderVK> device_holder,
25-
std::shared_ptr<TrackedObjectsVK> tracked_objects,
26-
std::shared_ptr<FenceWaiterVK> fence_waiter)
25+
std::shared_ptr<TrackedObjectsVK> tracked_objects)
2726
: CommandBuffer(std::move(context)),
2827
device_holder_(std::move(device_holder)),
29-
tracked_objects_(std::move(tracked_objects)),
30-
fence_waiter_(std::move(fence_waiter)) {}
28+
tracked_objects_(std::move(tracked_objects)) {}
3129

3230
CommandBufferVK::~CommandBufferVK() = default;
3331

@@ -212,4 +210,8 @@ void CommandBufferVK::InsertDebugMarker(std::string_view label) const {
212210
}
213211
}
214212

213+
DescriptorPoolVK& CommandBufferVK::GetDescriptorPool() const {
214+
return tracked_objects_->GetDescriptorPool();
215+
}
216+
215217
} // namespace impeller

impeller/renderer/backend/vulkan/command_buffer_vk.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "fml/status_or.h"
99
#include "impeller/base/backend_cast.h"
1010
#include "impeller/renderer/backend/vulkan/command_queue_vk.h"
11+
#include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h"
1112
#include "impeller/renderer/backend/vulkan/device_holder_vk.h"
1213
#include "impeller/renderer/backend/vulkan/texture_source_vk.h"
1314
#include "impeller/renderer/backend/vulkan/tracked_objects_vk.h"
@@ -81,18 +82,19 @@ class CommandBufferVK final
8182
// Visible for testing.
8283
bool IsTracking(const std::shared_ptr<const Texture>& texture) const;
8384

85+
// Visible for testing.
86+
DescriptorPoolVK& GetDescriptorPool() const;
87+
8488
private:
8589
friend class ContextVK;
8690
friend class CommandQueueVK;
8791

8892
std::weak_ptr<const DeviceHolderVK> device_holder_;
8993
std::shared_ptr<TrackedObjectsVK> tracked_objects_;
90-
std::shared_ptr<FenceWaiterVK> fence_waiter_;
9194

9295
CommandBufferVK(std::weak_ptr<const Context> context,
9396
std::weak_ptr<const DeviceHolderVK> device_holder,
94-
std::shared_ptr<TrackedObjectsVK> tracked_objects,
95-
std::shared_ptr<FenceWaiterVK> fence_waiter);
97+
std::shared_ptr<TrackedObjectsVK> tracked_objects);
9698

9799
// |CommandBuffer|
98100
void SetLabel(std::string_view label) const override;

impeller/renderer/backend/vulkan/context_vk.cc

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,12 @@
33
// found in the LICENSE file.
44

55
#include "impeller/renderer/backend/vulkan/context_vk.h"
6+
#include <thread>
7+
#include <unordered_map>
68

79
#include "fml/concurrent_message_loop.h"
810
#include "impeller/renderer/backend/vulkan/command_queue_vk.h"
11+
#include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h"
912
#include "impeller/renderer/backend/vulkan/render_pass_builder_vk.h"
1013
#include "impeller/renderer/render_target.h"
1114

@@ -30,6 +33,7 @@
3033
#include "impeller/renderer/backend/vulkan/command_pool_vk.h"
3134
#include "impeller/renderer/backend/vulkan/command_queue_vk.h"
3235
#include "impeller/renderer/backend/vulkan/debug_report_vk.h"
36+
#include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h"
3337
#include "impeller/renderer/backend/vulkan/fence_waiter_vk.h"
3438
#include "impeller/renderer/backend/vulkan/gpu_tracer_vk.h"
3539
#include "impeller/renderer/backend/vulkan/resource_manager_vk.h"
@@ -497,8 +501,22 @@ std::shared_ptr<CommandBuffer> ContextVK::CreateCommandBuffer() const {
497501
return nullptr;
498502
}
499503

504+
// look up a cached descriptor pool for the current frame and reuse it
505+
// if it exists, otherwise create a new pool.
506+
DescriptorPoolMap::iterator current_pool =
507+
cached_descriptor_pool_.find(std::this_thread::get_id());
508+
std::shared_ptr<DescriptorPoolVK> descriptor_pool;
509+
if (current_pool == cached_descriptor_pool_.end()) {
510+
descriptor_pool =
511+
(cached_descriptor_pool_[std::this_thread::get_id()] =
512+
std::make_shared<DescriptorPoolVK>(weak_from_this()));
513+
} else {
514+
descriptor_pool = current_pool->second;
515+
}
516+
500517
auto tracked_objects = std::make_shared<TrackedObjectsVK>(
501-
weak_from_this(), std::move(tls_pool), GetGPUTracer()->CreateGPUProbe());
518+
weak_from_this(), std::move(tls_pool), std::move(descriptor_pool),
519+
GetGPUTracer()->CreateGPUProbe());
502520
auto queue = GetGraphicsQueue();
503521

504522
if (!tracked_objects || !tracked_objects->IsValid() || !queue) {
@@ -517,10 +535,9 @@ std::shared_ptr<CommandBuffer> ContextVK::CreateCommandBuffer() const {
517535
tracked_objects->GetCommandBuffer());
518536

519537
return std::shared_ptr<CommandBufferVK>(new CommandBufferVK(
520-
shared_from_this(), //
521-
GetDeviceHolder(), //
522-
std::move(tracked_objects), //
523-
GetFenceWaiter() //
538+
shared_from_this(), //
539+
GetDeviceHolder(), //
540+
std::move(tracked_objects) //
524541
));
525542
}
526543

@@ -651,6 +668,7 @@ void ContextVK::InitializeCommonlyUsedShadersIfNeeded() const {
651668
}
652669

653670
void ContextVK::DisposeThreadLocalCachedResources() {
671+
cached_descriptor_pool_.erase(std::this_thread::get_id());
654672
command_pool_recycler_->Dispose();
655673
}
656674

impeller/renderer/backend/vulkan/context_vk.h

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
#include "flutter/fml/concurrent_message_loop.h"
1111
#include "flutter/fml/mapping.h"
1212
#include "flutter/fml/unique_fd.h"
13-
#include "fml/thread.h"
1413
#include "impeller/base/backend_cast.h"
1514
#include "impeller/base/strings.h"
1615
#include "impeller/core/formats.h"
@@ -40,6 +39,7 @@ class SurfaceContextVK;
4039
class GPUTracerVK;
4140
class DescriptorPoolRecyclerVK;
4241
class CommandQueueVK;
42+
class DescriptorPoolVK;
4343

4444
class ContextVK final : public Context,
4545
public BackendCast<ContextVK, Context>,
@@ -224,12 +224,17 @@ class ContextVK final : public Context,
224224
std::shared_ptr<const Capabilities> device_capabilities_;
225225
std::shared_ptr<FenceWaiterVK> fence_waiter_;
226226
std::shared_ptr<ResourceManagerVK> resource_manager_;
227+
std::shared_ptr<DescriptorPoolRecyclerVK> descriptor_pool_recycler_;
227228
std::shared_ptr<CommandPoolRecyclerVK> command_pool_recycler_;
228229
std::string device_name_;
229230
std::shared_ptr<fml::ConcurrentMessageLoop> raster_message_loop_;
230231
std::shared_ptr<GPUTracerVK> gpu_tracer_;
231-
std::shared_ptr<DescriptorPoolRecyclerVK> descriptor_pool_recycler_;
232232
std::shared_ptr<CommandQueue> command_queue_vk_;
233+
234+
using DescriptorPoolMap =
235+
std::unordered_map<std::thread::id, std::shared_ptr<DescriptorPoolVK>>;
236+
237+
mutable DescriptorPoolMap cached_descriptor_pool_;
233238
bool should_disable_surface_control_ = false;
234239
bool should_batch_cmd_buffers_ = false;
235240
std::vector<std::shared_ptr<CommandBuffer>> pending_command_buffers_;

impeller/renderer/backend/vulkan/descriptor_pool_vk_unittests.cc

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include "flutter/testing/testing.h" // IWYU pragma: keep.
66
#include "fml/closure.h"
77
#include "fml/synchronization/waitable_event.h"
8+
#include "impeller/renderer/backend/vulkan/command_buffer_vk.h"
89
#include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h"
910
#include "impeller/renderer/backend/vulkan/resource_manager_vk.h"
1011
#include "impeller/renderer/backend/vulkan/test/mock_vulkan.h"
@@ -123,5 +124,27 @@ TEST(DescriptorPoolRecyclerVKTest, ReclaimDropsDescriptorPoolIfSizeIsExceeded) {
123124
context->Shutdown();
124125
}
125126

127+
TEST(DescriptorPoolRecyclerVKTest, MultipleCommandBuffersShareDescriptorPool) {
128+
auto const context = MockVulkanContextBuilder().Build();
129+
130+
auto cmd_buffer_1 = context->CreateCommandBuffer();
131+
auto cmd_buffer_2 = context->CreateCommandBuffer();
132+
133+
CommandBufferVK& vk_1 = CommandBufferVK::Cast(*cmd_buffer_1);
134+
CommandBufferVK& vk_2 = CommandBufferVK::Cast(*cmd_buffer_2);
135+
136+
EXPECT_EQ(&vk_1.GetDescriptorPool(), &vk_2.GetDescriptorPool());
137+
138+
// Resetting resources creates a new pool.
139+
context->DisposeThreadLocalCachedResources();
140+
141+
auto cmd_buffer_3 = context->CreateCommandBuffer();
142+
CommandBufferVK& vk_3 = CommandBufferVK::Cast(*cmd_buffer_3);
143+
144+
EXPECT_NE(&vk_1.GetDescriptorPool(), &vk_3.GetDescriptorPool());
145+
146+
context->Shutdown();
147+
}
148+
126149
} // namespace testing
127150
} // namespace impeller

impeller/renderer/backend/vulkan/surface_context_vk.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ std::unique_ptr<Surface> SurfaceContextVK::AcquireNextSurface() {
8686
impeller::PipelineLibraryVK::Cast(*pipeline_library)
8787
.DidAcquireSurfaceFrame();
8888
}
89-
parent_->GetCommandPoolRecycler()->Dispose();
89+
parent_->DisposeThreadLocalCachedResources();
9090
parent_->GetResourceAllocator()->DebugTraceMemoryStatistics();
9191
return surface;
9292
}

impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
#include "flutter/testing/testing.h" // IWYU pragma: keep
66
#include "gtest/gtest.h"
7+
#include "impeller/renderer/backend/vulkan/command_pool_vk.h"
78
#include "impeller/renderer/backend/vulkan/test/mock_vulkan.h"
89
#include "vulkan/vulkan_enums.hpp"
910

impeller/renderer/backend/vulkan/tracked_objects_vk.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,17 @@
44

55
#include "impeller/renderer/backend/vulkan/tracked_objects_vk.h"
66

7+
#include "impeller/renderer/backend/vulkan/command_pool_vk.h"
78
#include "impeller/renderer/backend/vulkan/gpu_tracer_vk.h"
89

910
namespace impeller {
1011

1112
TrackedObjectsVK::TrackedObjectsVK(
1213
const std::weak_ptr<const ContextVK>& context,
1314
const std::shared_ptr<CommandPoolVK>& pool,
15+
std::shared_ptr<DescriptorPoolVK> descriptor_pool,
1416
std::unique_ptr<GPUProbe> probe)
15-
: desc_pool_(context), probe_(std::move(probe)) {
17+
: desc_pool_(std::move(descriptor_pool)), probe_(std::move(probe)) {
1618
if (!pool) {
1719
return;
1820
}
@@ -78,7 +80,7 @@ vk::CommandBuffer TrackedObjectsVK::GetCommandBuffer() const {
7880
}
7981

8082
DescriptorPoolVK& TrackedObjectsVK::GetDescriptorPool() {
81-
return desc_pool_;
83+
return *desc_pool_;
8284
}
8385

8486
GPUProbe& TrackedObjectsVK::GetGPUProbe() const {

impeller/renderer/backend/vulkan/tracked_objects_vk.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ class TrackedObjectsVK {
2020
public:
2121
explicit TrackedObjectsVK(const std::weak_ptr<const ContextVK>& context,
2222
const std::shared_ptr<CommandPoolVK>& pool,
23+
std::shared_ptr<DescriptorPoolVK> descriptor_pool,
2324
std::unique_ptr<GPUProbe> probe);
2425

2526
~TrackedObjectsVK();
@@ -43,7 +44,7 @@ class TrackedObjectsVK {
4344
GPUProbe& GetGPUProbe() const;
4445

4546
private:
46-
DescriptorPoolVK desc_pool_;
47+
std::shared_ptr<DescriptorPoolVK> desc_pool_;
4748
// `shared_ptr` since command buffers have a link to the command pool.
4849
std::shared_ptr<CommandPoolVK> pool_;
4950
vk::UniqueCommandBuffer buffer_;

0 commit comments

Comments
 (0)