Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Jul 12, 2023

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Jul 11, 2023

flutter/flutter#129392

Buffer allocations on worker threads can interefere with allocations on the main thread. Since all buffers that we allocate on the main thread are discarded after a frame, we can use a ring buffer of VmaPool objects with a linear allocate bit to get faster allocations.

This works for buffers because they are likely to be suballocated, so we can continue to reuse the same memory over and over. Whereas most of our textures require dedicated allocations.

In order to support this, we need the allocator to track an incrementing frame counter (actually this would probably be useful for the context to do in general, but we'll get there eventually).

image

@flutter-dashboard
Copy link

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.

@@ -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
Member Author

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.

@@ -27,11 +27,14 @@ class AllocatorVK final : public Allocator {

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

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.

@jonahwilliams
Copy link
Member Author

Man I just cannot figure out why this CI machine doesn't have any of these memory types.

@@ -93,6 +93,10 @@ AllocatorVK::AllocatorVK(std::weak_ptr<Context> context,
VALIDATION_LOG << "Could not create memory allocator";
return;
}
for (auto i = 0u; i < kPoolCount; i++) {
created_buffer_pools_ &=
Copy link
Member Author

Choose a reason for hiding this comment

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

Making this optional for now as the host unittests don't seem to be able to create the same buffer pools that real devices do.

@chinmaygarde chinmaygarde changed the title [Impeller] allocate buffers out of a pool on the raster thread. [Impeller] Allocate buffers out of a pool on the raster thread. Jul 12, 2023
@@ -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

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
Member Author

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.

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

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
Member 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.

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?

@@ -182,7 +191,9 @@ static constexpr VkMemoryPropertyFlags ToVKTextureMemoryPropertyFlags(
bool supports_memoryless_textures) {
switch (mode) {
case StorageMode::kHostVisible:
return VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT;
return VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT |
Copy link
Member

Choose a reason for hiding this comment

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

I know this is just modifying existing code, but can we use vk::MemoryPropertyFlagBits here (the C++ variant)?

Copy link
Member Author

Choose a reason for hiding this comment

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

how do I convert the vk::Flag type back to the C bits for VMA?

Copy link
Member

Choose a reason for hiding this comment

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

You can just static cast it to the underlying type. The header says enum class MemoryPropertyFlagBits : VkMemoryPropertyFlags. You can even use decltype for it. But your call as this isn't how the code works now.

Copy link
Member Author

Choose a reason for hiding this comment

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

ahh makes sense

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 12, 2023
@auto-submit auto-submit bot merged commit 8d03c4f into flutter:main Jul 12, 2023
28 checks passed
@jonahwilliams jonahwilliams deleted the buffer_queue branch July 12, 2023 23:19
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 13, 2023
…130496)

Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions)

flutter/engine@16e2ab7...b408036

2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631)
2023-07-13 ian@hixie.ch Make GOMA state automatic by default (flutter/engine#43584)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630)
2023-07-13 chinmaygarde@google.com [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628)
2023-07-13 chinmaygarde@google.com [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621)
2023-07-13 jonahwilliams@google.com [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388)
2023-07-12 jonahwilliams@google.com [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb
  fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
...
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 13, 2023
…lutter#130496)

Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions)

flutter/engine@16e2ab7...b408036

2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631)
2023-07-13 ian@hixie.ch Make GOMA state automatic by default (flutter/engine#43584)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630)
2023-07-13 chinmaygarde@google.com [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628)
2023-07-13 chinmaygarde@google.com [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621)
2023-07-13 jonahwilliams@google.com [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388)
2023-07-12 jonahwilliams@google.com [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb
  fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
...
kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
…ter#43564)

flutter/flutter#129392

Buffer allocations on worker threads can interefere with allocations on the main thread. Since all buffers that we allocate on the main thread are discarded after a frame, we can use a ring buffer of VmaPool objects with a linear allocate bit to get faster allocations.

This works for buffers because they are likely to be suballocated, so we can continue to reuse the same memory over and over. Whereas most of our textures require dedicated allocations.

In order to support this, we need the allocator to track an incrementing frame counter (actually this would probably be useful for the context to do in general, but we'll get there eventually).

![image](https://github.com/flutter/engine/assets/8975114/4a485e2b-c557-4567-9266-a725fc1448db)
harryterkelsen pushed a commit to harryterkelsen/engine that referenced this pull request Jul 20, 2023
…ter#43564)

flutter/flutter#129392

Buffer allocations on worker threads can interefere with allocations on the main thread. Since all buffers that we allocate on the main thread are discarded after a frame, we can use a ring buffer of VmaPool objects with a linear allocate bit to get faster allocations.

This works for buffers because they are likely to be suballocated, so we can continue to reuse the same memory over and over. Whereas most of our textures require dedicated allocations.

In order to support this, we need the allocator to track an incrementing frame counter (actually this would probably be useful for the context to do in general, but we'll get there eventually).

![image](https://github.com/flutter/engine/assets/8975114/4a485e2b-c557-4567-9266-a725fc1448db)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
No open projects
Archived in project
2 participants