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

[Impeller] Allocate buffers out of a pool on the raster thread. #43564

Merged
auto-submit merged 9 commits into from
Jul 12, 2023
Merged

[Impeller] Allocate buffers out of a pool on the raster thread. #43564

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8590d30
[Impeller] allocate buffers out of a pool on the raster thread.
Jul 11, 2023
866761f
fall back to host visible only
Jul 11, 2023
1a01926
wip testing
Jul 11, 2023
c85a337
testing
Jul 11, 2023
3947f81
++
Jul 11, 2023
b8bee47
++
Jul 11, 2023
9d4d44a
++
Jul 11, 2023
4efb6a8
use context didAcquireSurfaceFrame
Jul 12, 2023
10d29cc
use c++ types
Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[Impeller] allocate buffers out of a pool on the raster thread.
  • Loading branch information
jonahwilliams committed Jul 11, 2023
commit 8590d305ec01967d99bf7f5627517e77e4d813d9
2 changes: 2 additions & 0 deletions impeller/core/allocator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,6 @@ uint16_t Allocator::MinimumBytesPerRow(PixelFormat format) const {
return BytesPerPixelForPixelFormat(format);
}

void Allocator::IncrementFrame() {}

} // namespace impeller
4 changes: 4 additions & 0 deletions impeller/core/allocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ class Allocator {

virtual ISize GetMaxTextureSizeSupported() const = 0;

/// @brief Increment an internal frame used to cycle through a ring buffer of
/// allocation pools.
virtual void IncrementFrame();

protected:
Allocator();

Expand Down
78 changes: 78 additions & 0 deletions impeller/renderer/backend/vulkan/allocator_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ AllocatorVK::AllocatorVK(std::weak_ptr<Context> context,
VALIDATION_LOG << "Could not create memory allocator";
return;
}
for (auto i = 0u; i < 3u; i++) {
if (!CreateBufferPool(allocator, &staging_buffer_pools_[i])) {
return;
}
}
allocator_ = allocator;
supports_memoryless_textures_ = capabilities.SupportsMemorylessTextures();
is_valid_ = true;
Expand All @@ -101,6 +106,11 @@ AllocatorVK::AllocatorVK(std::weak_ptr<Context> context,
AllocatorVK::~AllocatorVK() {
TRACE_EVENT0("impeller", "DestroyAllocatorVK");
if (allocator_) {
for (auto i = 0u; i < 3u; i++) {
if (staging_buffer_pools_[i]) {
::vmaDestroyPool(allocator_, staging_buffer_pools_[i]);
}
}
::vmaDestroyAllocator(allocator_);
}
}
Expand Down Expand Up @@ -210,6 +220,22 @@ static constexpr VkMemoryPropertyFlags ToVKBufferMemoryPropertyFlags(
FML_UNREACHABLE();
}

static VmaAllocationCreateFlags ToVmaAllocationBufferCreateFlags(
StorageMode mode) {
VmaAllocationCreateFlags flags = 0;
switch (mode) {
case StorageMode::kHostVisible:
flags |= VMA_ALLOCATION_CREATE_HOST_ACCESS_SEQUENTIAL_WRITE_BIT;
flags |= VMA_ALLOCATION_CREATE_MAPPED_BIT;
return flags;
case StorageMode::kDevicePrivate:
return flags;
case StorageMode::kDeviceTransient:
return flags;
}
FML_UNREACHABLE();
}

static VmaAllocationCreateFlags ToVmaAllocationCreateFlags(StorageMode mode,
bool is_texture,
size_t size) {
Expand Down Expand Up @@ -388,6 +414,11 @@ std::shared_ptr<Texture> AllocatorVK::OnCreateTexture(
return std::make_shared<TextureVK>(context_, std::move(source));
}

void AllocatorVK::IncrementFrame() {
frame_count_++;
raster_thread_id_ = std::this_thread::get_id();
Copy link
Contributor Author

@jonahwilliams jonahwilliams Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an assumption that the thread which invokves IncrementFrame is the raster thread and will do most of the allocation. This is important to keep worker thread allocations in the default pool.

}

// |Allocator|
std::shared_ptr<DeviceBuffer> AllocatorVK::OnCreateBuffer(
const DeviceBufferDescriptor& desc) {
Expand All @@ -410,6 +441,11 @@ std::shared_ptr<DeviceBuffer> AllocatorVK::OnCreateBuffer(
ToVKBufferMemoryPropertyFlags(desc.storage_mode);
allocation_info.flags = ToVmaAllocationCreateFlags(
desc.storage_mode, /*is_texture=*/false, desc.size);
allocation_info.flags = ToVmaAllocationBufferCreateFlags(desc.storage_mode);
if (desc.storage_mode == StorageMode::kHostVisible &&
raster_thread_id_ == std::this_thread::get_id()) {
allocation_info.pool = staging_buffer_pools_[frame_count_ % 3u];
}

VkBuffer buffer = {};
VmaAllocation buffer_allocation = {};
Expand Down Expand Up @@ -437,4 +473,46 @@ std::shared_ptr<DeviceBuffer> AllocatorVK::OnCreateBuffer(
);
}

// static
bool AllocatorVK::CreateBufferPool(VmaAllocator allocator, VmaPool* pool) {
vk::BufferCreateInfo buffer_info;
buffer_info.usage = vk::BufferUsageFlagBits::eVertexBuffer |
vk::BufferUsageFlagBits::eIndexBuffer |
vk::BufferUsageFlagBits::eUniformBuffer |
vk::BufferUsageFlagBits::eStorageBuffer |
vk::BufferUsageFlagBits::eTransferSrc |
vk::BufferUsageFlagBits::eTransferDst;
buffer_info.size = 1u; // doesn't matter
buffer_info.sharingMode = vk::SharingMode::eExclusive;
auto buffer_info_native =
static_cast<vk::BufferCreateInfo::NativeType>(buffer_info);

VmaAllocationCreateInfo allocation_info = {};
allocation_info.usage = VMA_MEMORY_USAGE_AUTO;
allocation_info.preferredFlags =
ToVKBufferMemoryPropertyFlags(StorageMode::kHostVisible);
allocation_info.flags =
ToVmaAllocationBufferCreateFlags(StorageMode::kHostVisible);

uint32_t memTypeIndex;
auto result = vk::Result{vmaFindMemoryTypeIndexForBufferInfo(
allocator, &buffer_info_native, &allocation_info, &memTypeIndex)};
if (result != vk::Result::eSuccess) {
VALIDATION_LOG << "Could not find memory type for buffer pool.";
return false;
}

VmaPoolCreateInfo pool_create_info = {};
pool_create_info.memoryTypeIndex = memTypeIndex;
pool_create_info.flags = VMA_POOL_CREATE_IGNORE_BUFFER_IMAGE_GRANULARITY_BIT |
VMA_POOL_CREATE_LINEAR_ALGORITHM_BIT;

result = vk::Result{vmaCreatePool(allocator, &pool_create_info, pool)};
Copy link
Member

@chinmaygarde chinmaygarde Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pools, allocators, and allocations are getting pervasive enough that not having RAII wrappers is getting hard to reason about. In this patch or another if you prefer, can we wrap these in fml::UniqueObject please?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I could make one for the allocator, but the pools also need an allocator for disposal and it doesn't look like fml::UniqueObject supports a userdata like ptr I could use for that. Is that a reasonable update you could make to support disposing pools?

if (result != vk::Result::eSuccess) {
VALIDATION_LOG << "Could not create buffer pool.";
return false;
}
return true;
}

} // namespace impeller
8 changes: 8 additions & 0 deletions impeller/renderer/backend/vulkan/allocator_vk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,14 @@ class AllocatorVK final : public Allocator {

fml::RefPtr<vulkan::VulkanProcTable> vk_;
VmaAllocator allocator_ = {};
VmaPool staging_buffer_pools_[3] = {};
Copy link
Contributor Author

@jonahwilliams jonahwilliams Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 seems like a reasonable number since it matches the most common swapchain count. In theory with a higher swapchain count a buffer from a pool could still be in use by the time we come back around, but that wouldn't actually cause any issues beyond higher memory use.

std::weak_ptr<Context> context_;
std::weak_ptr<DeviceHolder> device_holder_;
ISize max_texture_size_;
bool is_valid_ = false;
bool supports_memoryless_textures_ = false;
uint32_t frame_count_ = 0;
std::thread::id raster_thread_id_;

AllocatorVK(std::weak_ptr<Context> context,
uint32_t vulkan_api_version,
Expand All @@ -45,6 +48,9 @@ class AllocatorVK final : public Allocator {
// |Allocator|
bool IsValid() const;

// |Allocator|
void IncrementFrame() override;

// |Allocator|
std::shared_ptr<DeviceBuffer> OnCreateBuffer(
const DeviceBufferDescriptor& desc) override;
Expand All @@ -56,6 +62,8 @@ class AllocatorVK final : public Allocator {
// |Allocator|
ISize GetMaxTextureSizeSupported() const override;

static bool CreateBufferPool(VmaAllocator allocator, VmaPool* pool);

FML_DISALLOW_COPY_AND_ASSIGN(AllocatorVK);
};

Expand Down
1 change: 1 addition & 0 deletions impeller/renderer/backend/vulkan/swapchain_impl_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ SwapchainImplVK::AcquireResult SwapchainImplVK::AcquireNextDrawable() {
}

const auto& context = ContextVK::Cast(*context_strong);
context.GetResourceAllocator()->IncrementFrame();

current_frame_ = (current_frame_ + 1u) % synchronizers_.size();

Expand Down
1 change: 1 addition & 0 deletions shell/common/snapshot_controller_impeller.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ sk_sp<DlImage> SnapshotControllerImpeller::DoMakeRasterSnapshot(
impeller::Picture picture = dispatcher.EndRecordingAsPicture();
auto context = GetDelegate().GetAiksContext();
if (context) {
context->GetContext()->GetResourceAllocator()->IncrementFrame();
Copy link
Member

@chinmaygarde chinmaygarde Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This depends on the caller explicitly telling Impeller that a frame was rendered. I'd like to keep caller responsibility w.r.t this to a minimum. in ContextVK, I keyed the surface acquisition as being a close enough proxy for frame generation. Thats how the pipeline library knows how many ticks have happened so it can decide when to persist the PSO cache. I suggest we do the same here. See ContextVK::AcquireNextSurface.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh that is just what I'm looking for, thank you.

auto max_size = context->GetContext()
->GetResourceAllocator()
->GetMaxTextureSizeSupported();
Expand Down