[Impeller] Use separate pool for staging buffers. #43325

jonahwilliams · 2023-06-29T00:44:21Z

Based off of https://github.com/flutter/engine/pull/43313/files

flutter-dashboard · 2023-06-29T00:44:24Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jonahwilliams · 2023-06-29T00:45:30Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+    auto image_upload = desc.usage_hint == UsageHint::kImageUpload;
+    alloc_nfo.flags = ToVmaAllocationCreateFlags(
+        desc.storage_mode, /*is_texture=*/true,
+        image_upload ? 0u : desc.GetByteSizeOfBaseMipLevel());


Sort of hacky, but if we want to allocate in the specific pool ... we can't ask for dedicated memory. Makes sense

This may be a bit more clear if you collapse it to one branch instead of 2 identical branches next to each other.

jonahwilliams · 2023-06-29T04:36:43Z

This is missing some checks that the usage is correct and currently crashes on an S10. Locally it might be marginally faster, and I still see long stalls on that device.

That said, the image decompression competing with raster frames is improved on my Pixel 6.

chinmaygarde · 2023-06-29T15:09:45Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+constexpr size_t kImageSizeThresholdForDedicatedMemoryAllocation =
+    4 * 1024 * 1024;
+
+static bool CreateBufferPool(VmaAllocator allocator, VmaPool* pool) {


IMO, we should use fml::UniqueObject for pools, allocators, textures, and buffers from VMA. I took a closer look at the early returns and it seems to be fine but its easy to mess up and leak resources especially when it comes to error conditions. And we already have an RAII wrapper.

In a later patch is fine of course. Of file a followup and we can pick it up later so we don't forget.

chinmaygarde · 2023-06-29T15:10:22Z

impeller/core/formats.h


 std::string TextureUsageMaskToString(TextureUsageMask mask);

+enum class UsageHint {


ResourceUsageHint perhaps? This is a bit ambiguous.

Maybe rather than a hint it should be more explict as "ResourceUsage", because I think we should also adjust the usage flags to see if that helps. For example ResourceUsage.stagingBuffer should only be a transferSrc, and doesn't need random access, et cetera.

we can have a "general" category that retains the current set of features

chinmaygarde · 2023-06-29T15:14:37Z

impeller/core/formats.h


+enum class UsageHint {
+  /// @brief Texture or buffer is being used during the raster workload.
+  kRasterWorkload,


How about kTransient and kPermanent? You expect to allocate the former in or near a frame workload and the latter has a longer lifetime.

My sense is that the current names betray this being used for a very Flutter as it works today use case.

chinmaygarde · 2023-06-29T15:18:06Z

impeller/renderer/backend/vulkan/allocator_vk.cc


+// Maximum size to use VMA image suballocation. Any allocation greater than or
+// equal to this value will use a dedicated VkDeviceMemory.
+constexpr size_t kImageSizeThresholdForDedicatedMemoryAllocation =


As mentioned in #43313 (comment), let's put the tunables in a separate file so everything is available at a glance?

chinmaygarde · 2023-06-29T15:18:11Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+
+  VmaPoolCreateInfo poolCreateInfo = {};
+  poolCreateInfo.memoryTypeIndex = memTypeIndex;
+  poolCreateInfo.blockSize = 128ull * 1024 * 1024;


As mentioned in #43313 (comment), let's put the tunables in a separate file so everything is available at a glance?

chinmaygarde · 2023-06-29T15:18:21Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+    // Create a pool that can have at most 2 blocks, 128 MiB each.
+    VmaPoolCreateInfo poolCreateInfo = {};
+    poolCreateInfo.memoryTypeIndex = memTypeIndex;
+    poolCreateInfo.blockSize = 128ull * 1024 * 1024;


As mentioned in #43313 (comment), let's put the tunables in a separate file so everything is available at a glance?

chinmaygarde · 2023-06-29T15:19:09Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+  poolCreateInfo.memoryTypeIndex = memTypeIndex;
+  poolCreateInfo.blockSize = 128ull * 1024 * 1024;
+
+  auto result = vk::Result{vmaCreatePool(allocator, &poolCreateInfo, pool)};


These pools are sparse to start with right? I suppose we'll find out :)

Yea, I think so. I'm not sure what blockSize = 0 is doing. Does it realloc if it gets too big?

gaaclarke

Oh boy, this definitely complicates everything but I think this is the way forward, nice work. At the very least a pool should be eliminating the overhead of selecting the memory to use. I think we might be able to squeeze out more performance by using a linear algorithm.

gaaclarke · 2023-06-29T16:19:26Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+  FML_UNREACHABLE();
+}
+
+vk::Flags<vk::BufferUsageFlagBits> VmaBufferUsageFlags(UsageHint usage) {


Nice, from my understanding this tidying up can make a difference.

gaaclarke · 2023-06-29T16:23:08Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+  VkResult res = vmaFindMemoryTypeIndexForBufferInfo(
+      allocator, &buffer_info_native, &allocation_info, &memTypeIndex);
+
+  VmaPoolCreateInfo poolCreateInfo = {};


Did you try out VMA_POOL_CREATE_LINEAR_ALGORITHM_BIT? The API is a bit cheeky in that it doesn't tell you what happens down below but considering these are just living for one frame and getting thrown away it may work.

gaaclarke · 2023-06-29T16:27:41Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+  VkResult res = vmaFindMemoryTypeIndexForBufferInfo(
+      allocator, &buffer_info_native, &allocation_info, &memTypeIndex);
+
+  VmaPoolCreateInfo poolCreateInfo = {};


Do we want to supply a blockSize?

gaaclarke · 2023-06-29T16:28:39Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+  poolCreateInfo.memoryTypeIndex = memTypeIndex;
+  poolCreateInfo.blockSize = 128ull * 1024 * 1024;
+
+  auto result = vk::Result{vmaCreatePool(allocator, &poolCreateInfo, pool)};


Yea, I think so. I'm not sure what blockSize = 0 is doing. Does it realloc if it gets too big?

gaaclarke · 2023-06-29T16:29:49Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+  FML_UNREACHABLE();
+}
+
+static bool CreateBufferPool(VmaAllocator allocator,


nit: We probably should be using fml::status instead of bool for returning errors since we have it.

gaaclarke · 2023-06-29T16:33:03Z

impeller/renderer/backend/vulkan/allocator_vk.cc

+    auto image_upload = desc.usage_hint == UsageHint::kImageUpload;
+    alloc_nfo.flags = ToVmaAllocationCreateFlags(
+        desc.storage_mode, /*is_texture=*/true,
+        image_upload ? 0u : desc.GetByteSizeOfBaseMipLevel());


This may be a bit more clear if you collapse it to one branch instead of 2 identical branches next to each other.

gaaclarke · 2023-06-29T16:34:19Z

impeller/renderer/backend/vulkan/allocator_vk.h


  fml::RefPtr<vulkan::VulkanProcTable> vk_;
  VmaAllocator allocator_ = {};
+  VmaPool raster_buffer_pool_ = {};


When we go forward with this we should have a docstring for the different pools. This is complicated enough that making it clear is helpful.

chinmaygarde · 2023-07-05T20:21:47Z

Sorry for the delay, I just lgtmed the dependent PR.

jonahwilliams · 2023-07-05T23:54:00Z

Updated.

I've backed out the change for texture allocation because I can't get a clear signal on whether or not that helps. At least moving the buffer allocation to one pool allows us to ignore granularity which can help make allocations faster. But its all sort of nebulous imo. I'm putting VMA down for a while after this.

jonahwilliams added 4 commits June 28, 2023 12:52

[Impeller] allocate fewer textures in dedicated memory.

b6b5f80

Update allocator_vk.cc

0798fe1

format

4826724

[Impeller] use separate pools for image upload work.

75c4015

flutter-dashboard bot added the needs tests label Jun 29, 2023

github-actions bot added the e: impeller label Jun 29, 2023

jonahwilliams commented Jun 29, 2023

View reviewed changes

jonahwilliams requested review from chinmaygarde and gaaclarke June 29, 2023 00:53

chinmaygarde reviewed Jun 29, 2023

View reviewed changes

chinmaygarde assigned jonahwilliams Jun 29, 2023

chinmaygarde changed the title ~~[Impeller] use separate buffer pools and a dedicated texture pool for uploads.~~ [Impeller] Use separate buffer pools and a dedicated texture pool for uploads. Jun 29, 2023

jonahwilliams added 2 commits June 29, 2023 09:01

wip

b2a7afc

merge

82063b9

gaaclarke reviewed Jun 29, 2023

View reviewed changes

jonahwilliams mentioned this pull request Jun 29, 2023

[Impeller] Check for lazy memory support. #43339

Merged

++

cb1c12d

jonahwilliams added 5 commits July 5, 2023 15:44

Merge branch 'main' of github.com:flutter/engine into separate_pools

b3e68df

++

a87fd1b

backout to just buffer pool.

eb5f50f

++

d29da31

++

b27390a

jonahwilliams changed the title ~~[Impeller] Use separate buffer pools and a dedicated texture pool for uploads.~~ [Impeller] Use separate pool for staging buffers. Jul 5, 2023

jonahwilliams requested a review from chinmaygarde July 5, 2023 23:54

Jonah Williams added 2 commits July 6, 2023 09:59

Update allocator_vk.cc

7d62f0a

++

d45fd78

jonahwilliams added the Work in progress (WIP) Not ready (yet) for review! label Jul 6, 2023

jonahwilliams added 4 commits July 6, 2023 19:24

Merge branch 'master' of github.com:flutter/engine into separate_pools

e70012f

remove device local bit when falling back.

10be3d5

Drafty stuff

680906d

++

5f79c52

jonahwilliams closed this Jul 10, 2023


		std::string TextureUsageMaskToString(TextureUsageMask mask);

		enum class UsageHint {

[Impeller] Use separate pool for staging buffers. #43325

[Impeller] Use separate pool for staging buffers. #43325

Uh oh!

Conversation

jonahwilliams commented Jun 29, 2023

Uh oh!

flutter-dashboard bot commented Jun 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jun 29, 2023

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!

Choose a reason for hiding this comment

Uh oh!

gaaclarke 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!

chinmaygarde commented Jul 5, 2023

Uh oh!

jonahwilliams commented Jul 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants