[Impeller] moved to one staging buffer pool

[gaaclarke]

I measured the pool usage while using the wonderous app after having consolidated the pools by printing capturing the pool usage after every call of vmaCreateBuffer. The max number of allocations ever seen in this pool were 1288704 B. Each of these pools are 32 MB. So this change removes 64MB from the baseline memory usage of a Vulkan Flutter engine. Worse case for a swapchain of 3 would be roughly 3 * 1.2 MB which is well within the limits of the 32 MB buffer.

Using one pool also increases the likelihood that a buffer will have been more recently used which leads to better memory performance.

This is safe to do since buffers will only be placed back into the pool after they are disposed of.

patch used for measuring

--- a/impeller/renderer/backend/vulkan/allocator_vk.cc
+++ b/impeller/renderer/backend/vulkan/allocator_vk.cc
@@ -474,6 +474,15 @@ std::shared_ptr<DeviceBuffer> AllocatorVK::OnCreateBuffer(
                                              &buffer_allocation_info  //
                                              )};
 
+  VmaStatistics pool_stats;
+  ::vmaGetPoolStatistics(allocator_.get(), staging_buffer_pool_.get().pool,
+                         &pool_stats);
+  static uint64_t s_max = 0;
+  if (pool_stats.allocationBytes > s_max) {
+    s_max = pool_stats.allocationBytes;
+    FML_LOG(ERROR) << "pool max: " << s_max;
+  }
+
   if (result != vk::Result::eSuccess) {
     VALIDATION_LOG << "Unable to allocate a device buffer: "
                    << vk::to_string(result);

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

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]

You need to remove the Linear allocted bit from the pool or else there is a chance to just endlessly leak memory (as allocation only happens at the end).

[jonahwilliams]

Specifically: https://github.com/flutter/engine/blob/main/impeller/renderer/backend/vulkan/allocator_vk.cc#L77

[gaaclarke]

You need to remove the Linear allocted bit from the pool or else there is a chance to just endlessly leak memory (as allocation only happens at the end).

Done. If we wanted to have a more optimal pool we could look into a ring buffer. Since in practice i'm seeing about 1/32 of the pool being used it's probably not hard for the algorithm to find space for new allocations.

[jonahwilliams]

ring buffer isn't something we can use for this, only VMA or Vulkan supported allocation strategies are reasonable.

[jonahwilliams]

LGTM

[gaaclarke]

ring buffer isn't something we can use for this, only VMA or Vulkan supported allocation strategies are reasonable.

The VMA documentation makes mention of using a ring buffer: https://gpuopen-librariesandsdks.github.io/VulkanMemoryAllocator/html/custom_memory_pools.html#linear_algorithm

I've read through it a few times and I'm guessing you have to implement it yourself on top of the pool? It isn't very clear.

[jonahwilliams]

That would be a ring buffer of VMA pools, each with a linear allocated bit set. That is the current implementation, which you are removing 😄

[gaaclarke]

That would be a ring buffer of VMA pools, each with a linear allocated bit set. That is the current implementation, which you are removing 😄

It's not that because it says the requirement is that the blockCount == 1, so it is talking about one pool acting as a ring buffer.

[jonahwilliams]

Ahh you got me! I think that might work except, what happens if we run out of memory?

[gaaclarke]

Ahh you got me! I think that might work except, what happens if we run out of memory?

We wouldn't in the ring buffer case because we are limited by the swapchain images we have. At worst case we would have swapchain_number of resources in memory. When the last frame is rendered all of that space will be given back to the ring. When writing overlaps reading in a ring buffer its a buffer overrun and since in practice it looks like we are using at most 1.2 MB, lets say thats per frame, we'd never get close to that point with a 32MB ring.

[jonahwilliams]

There is no bound on the amount of memory we can use per frame. A developer could create a picture with thousands of paths that leads to many MB of geometry. Anything is possible.

[auto-submit]

auto label is removed for flutter/engine, pr: 44172, due to - The status or check suite Mac mac_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.