[Impeller] Cache render target texture allocations across frames. #44527

jonahwilliams · 2023-08-09T03:27:00Z

When allocating a non-host visible texture for a render target, the allocator will hold onto a reference to this texture descriptor. On the immediate next frame, this texture is made available to replace requested allocations for new render target textures. if this texture is not used, then at the end of the next frame it is discarded.

This removes the vast majority of allocations in most flutter gallery and wonderous. This does not attempt to use different sized textures for the cache.

There are two caveats noted in the PR contents.

Fixes flutter/flutter#131515

impeller/core/allocator.cc

impeller/core/texture_descriptor.h

impeller/renderer/backend/vulkan/allocator_vk.cc

…still_hacky_cache

bdero · 2023-08-14T20:22:46Z

impeller/renderer/context.h

+  ///
+  ///        This may or may not indicate a new frame, or there may be multiple
+  ///        presents within a single frame due to platform view presence.
+  virtual void DidAcquireSurfaceFrame() const = 0;


On the surface, it doesn't seem like the HAL is a good place to cache user-allocated GPU resources, partly due to this frame concept we need to snake through the context in order to drive cache lifetimes. Would there be any downside to just letting applications choose when/how they want to reuse user-managed textures explicitly?

I don't think this new cache will be suitable for most renderers that will be built with Impeller. (Including EntityPass, as we continue to optimize -- especially if we want to use larger render target textures for rendering smaller passes, which would inevitably require viewport and sampling/UV adjustments in EntityPass).

We could add a convenient cache manager utility under Entity (or maybe core) that caches textures based on descriptor w/ a configurable lifetime, for example. This kind of thing would be driven by EntityPass.

is the context the HAL? and what is the application?

in terms of where does the cache live, we could definitely put it in entity pass. But I'm not sure how we live without the frame concept. I don't think its the case that 1 entity pass = 1 presentation cycle, or is it the case that the current cache is based around frames.

FYI @dnfield , does you AIKS canvas patch cause us to create multiple entity passes per frame (non-platform view)? or are impeller picutres always combined into a single entity pass?

Well, we can't store it in entity pass - but we could store it on the content context and access via entity pass.

is the context the HAL?

Yeah, impeller::Context is the central resource of the HAL, since you use it to spawn all GPU resources and behaviors. By HAL I mean pretty much everything under impeller/renderer, or our equivalent of gfx_hal or WebGPU.

and what is the application?

Renderers built atop the HAL. EntityPass, Flutter GPU renderers, and my stupid side projects 😛.

But I'm not sure how we live without the frame concept. I don't think its the case that 1 entity pass = 1 presentation cycle, or is it the case that the current cache is based around frames.

One call to EntityPass::Render() should happen per-platform view per-frame.

does you AIKS canvas patch cause us to create multiple entity passes per frame (non-platform view)?

Oh, do you mean the DrawPicture stuff? DrawPicture merges a cloned EntityPass from a picture into another and won't result in EntityPass::Render() being called more than once per-platform view per-frame.

Ah, you probably mean #44248.

SGTM, I'll move the allocation cache to a structure on the content context. I think I'll adjust the render target constructors a bit so that they also accept an allocator - and I can wrap the context allocator in a caching content_context allocator?

TBH the RenderTarget::CreateOffscreen[MSAA] static factories wound up being super Entities-specific in the end and belong in something like ContentContext anyhow. Moving those factories up into Entities and passing in some optional Entities-specific cache through EntityPass CreateRenderTarget seems like a potentially good route.

bdero · 2023-08-14T20:35:56Z

impeller/renderer/backend/metal/surface_mtl.mm

    VALIDATION_LOG << "Could not acquire current drawable.";
    return nullptr;
  }
+  context->DidAcquireSurfaceFrame();


Did we end up making the Impeller context unique per-surface/platform view?

bdero · 2023-08-14T21:20:21Z

impeller/renderer/backend/metal/surface_mtl.mm

  if (!context) {
    return false;
  }
+  context->DidFinishSurfaceFrame();


Do we need to call these in the playground and between golden PNG generation in order to not leak/risk GPU OOM?

bdero · 2023-08-14T21:25:51Z

impeller/renderer/context.h

+  ///
+  ///        This may or may not indicate a new frame, or there may be multiple
+  ///        presents within a single frame due to platform view presence.
+  virtual void DidFinishSurfaceFrame() const = 0;


Should we call these BeginFrame() and EndFrame(), since surfaces aren't necessarily involved here?

We should also document that GPU resources may leak if they aren't called properly.

dnfield · 2023-08-14T21:39:31Z

impeller/core/allocator.cc

+  data_to_recycle_.clear();
+  data_to_recycle_.insert(data_to_recycle_.end(), retain.begin(), retain.end());


Suggested change

data_to_recycle_.clear();

data_to_recycle_.insert(data_to_recycle_.end(), retain.begin(), retain.end());

data_to_recycle_.swap(retain);

impeller/core/allocator.cc

impeller/core/texture_descriptor.h

dnfield · 2023-08-14T21:41:38Z

impeller/core/texture_descriptor.h

+  /// @brief Whether the [other] texture descriptor has the same properties.
+  ///
+  ///        This is used to check compatibility of already allocated textures
+  ///        in the render target cache..
+  constexpr bool IsCompatibleWith(const TextureDescriptor& other) const {
+    return size == other.size &&                  //
+           storage_mode == other.storage_mode &&  //
+           format == other.format &&              //
+           usage == other.usage &&                //
+           sample_count == other.sample_count &&  //
+           type == other.type;                    //
+  }


maybe just constexpr bool operator==(?

jonahwilliams · 2023-08-15T01:36:53Z

Marking WIP while I move the caching around.

jonahwilliams · 2023-08-15T04:05:52Z

Based on @bdero 's feedback I've moved the caching to EntityPass::Render. This works as expected in all backends, and Ill follow up tomorrow to ensure that this keeps working with @dnfield s aiks canvas patches.

dnfield · 2023-08-15T05:00:18Z

There should only be one EntityPass::Render per frame after my patch as well. But we should probably document this and DCHECK it somehow if possible.

jonahwilliams · 2023-08-15T17:41:29Z

There will be more than one entity pass per frame if there are platform views, that is fine as we're not attempting to recycle the swapchain textures.

bdero

LGTM, just minor nits!

impeller/entity/render_target_cache_unittests.cc

bdero · 2023-08-15T19:56:14Z

impeller/entity/render_target_cache.h

+///        allocated texture data for one frame.
+///
+///        Any textures unused after a frame are immediately discarded.
+class RenderTargetCache : public RenderTargetAllocator {


Really like this idea!

bdero · 2023-08-15T20:00:07Z

impeller/core/texture_descriptor.h

+           sample_count != other.sample_count ||          //
+           type != other.type ||                          //
+           compression_type != other.compression_type ||  //
+           mip_count != other.mip_count;


Could this just be return !(*this == other)?

I guess this is a struct ... does that work? I don't know C++ 😆

Yeah it should work; classes are just structs with private members by default. :p

Oh, duh. I thought you were saying to do a ptr comparison or something like an implict equal operator. I didn't realize you were specifically talking about using the == operation to define the != operation. Makes sense!

Co-authored-by: Brandon DeRosier <x@bdero.me>

…rames. (flutter/engine#44527)

…132612) flutter/engine@7409ce4...659cdfc 2023-08-16 jonahwilliams@google.com [Impeller] Cache render target texture allocations across frames. (flutter/engine#44527) 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 jsimmons@google.com,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

…ayouts (#45088) This refactor makes recycling render passes easier since it starts to split out setting the layout of textures with creating the render pass description. I didn't fully implement the split since it would technically be slower. Until we get caching of render passes working it would require us to calculate the attachment descriptions twice, once to make the render pass and once to set the texture layout. teases out some of the work from: #44527 in the service of implementing: #44861 issue: flutter/flutter#133182 The final split will have this function for recycled render passes: ```c++ void SetTextureLayouts(const RenderTarget& render_target, const std::shared_ptr<CommandBufferVK>& command_buffer) { for (const auto& [bind_point, color] : render_target.GetColorAttachments()) { SetTextureLayout(color, CreateAttachmentDescription(color, &Attachment::texture), command_buffer, &Attachment::texture); if (color.resolve_texture) { SetTextureLayout( color, CreateAttachmentDescription(color, &Attachment::resolve_texture), command_buffer, &Attachment::resolve_texture); } } if (auto depth = render_target.GetDepthAttachment(); depth.has_value()) { SetTextureLayout( depth.value(), CreateAttachmentDescription(depth.value(), &Attachment::texture), command_buffer, &Attachment::texture); } if (auto stencil = render_target.GetStencilAttachment(); stencil.has_value()) { SetTextureLayout( stencil.value(), CreateAttachmentDescription(stencil.value(), &Attachment::texture), command_buffer, &Attachment::texture); } } ``` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

…utter#44527) When allocating a non-host visible texture for a render target, the allocator will hold onto a reference to this texture descriptor. On the immediate next frame, this texture is made available to replace requested allocations for new render target textures. if this texture is not used, then at the end of the next frame it is discarded. This removes the vast majority of allocations in most flutter gallery and wonderous. This does not attempt to use different sized textures for the cache. There are two caveats noted in the PR contents. Fixes flutter/flutter#131515

…ayouts (flutter#45088) This refactor makes recycling render passes easier since it starts to split out setting the layout of textures with creating the render pass description. I didn't fully implement the split since it would technically be slower. Until we get caching of render passes working it would require us to calculate the attachment descriptions twice, once to make the render pass and once to set the texture layout. teases out some of the work from: flutter#44527 in the service of implementing: flutter#44861 issue: flutter/flutter#133182 The final split will have this function for recycled render passes: ```c++ void SetTextureLayouts(const RenderTarget& render_target, const std::shared_ptr<CommandBufferVK>& command_buffer) { for (const auto& [bind_point, color] : render_target.GetColorAttachments()) { SetTextureLayout(color, CreateAttachmentDescription(color, &Attachment::texture), command_buffer, &Attachment::texture); if (color.resolve_texture) { SetTextureLayout( color, CreateAttachmentDescription(color, &Attachment::resolve_texture), command_buffer, &Attachment::resolve_texture); } } if (auto depth = render_target.GetDepthAttachment(); depth.has_value()) { SetTextureLayout( depth.value(), CreateAttachmentDescription(depth.value(), &Attachment::texture), command_buffer, &Attachment::texture); } if (auto stencil = render_target.GetStencilAttachment(); stencil.has_value()) { SetTextureLayout( stencil.value(), CreateAttachmentDescription(stencil.value(), &Attachment::texture), command_buffer, &Attachment::texture); } } ``` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

jonahwilliams added 2 commits August 8, 2023 20:25

[Impeller] less hacky but still hacky cache.

7fcb5e5

++

d735350

github-actions bot added the e: impeller label Aug 9, 2023

jonahwilliams mentioned this pull request Aug 9, 2023

[Impeller] Implement pooling of render target textures. flutter/flutter#131515

Closed

jonahwilliams added 4 commits August 9, 2023 17:28

add memory barriers for re-used textures

6c62257

++

9cbe8a2

fix metal

7e96460

fix deallocation

9bbfa67

chinmaygarde changed the title ~~Less hacky but still hacky cache~~ [Impeller] Less hacky but still hacky cache Aug 11, 2023

chinmaygarde changed the title ~~[Impeller] Less hacky but still hacky cache~~ [Impeller] Less hacky but still hacky cache. Aug 11, 2023

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Aug 11, 2023

chinmaygarde assigned jonahwilliams Aug 11, 2023

jonahwilliams added 3 commits August 13, 2023 12:11

++

66d9aa6

Add unittests and cleanup

14e21a0

++

88e9d87

jonahwilliams changed the title ~~[Impeller] Less hacky but still hacky cache.~~ [Impeller] Cache render target texture allocations across frames. Aug 14, 2023

jonahwilliams commented Aug 14, 2023

View reviewed changes

impeller/core/allocator.cc Outdated Show resolved Hide resolved

jonahwilliams commented Aug 14, 2023

View reviewed changes

impeller/core/texture_descriptor.h Outdated Show resolved Hide resolved

jonahwilliams commented Aug 14, 2023

View reviewed changes

impeller/renderer/backend/vulkan/allocator_vk.cc Outdated Show resolved Hide resolved

jonahwilliams added 2 commits August 14, 2023 09:59

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

02d9698

…still_hacky_cache

++

4775a1e

jonahwilliams removed the Work in progress (WIP) Not ready (yet) for review! label Aug 14, 2023

++

40c57e7

jonahwilliams requested review from bdero, chinmaygarde and dnfield August 14, 2023 17:35

jonahwilliams marked this pull request as ready for review August 14, 2023 17:35

excluded

386c401

bdero reviewed Aug 14, 2023

View reviewed changes

dnfield reviewed Aug 14, 2023

View reviewed changes

jonahwilliams added 2 commits August 14, 2023 18:35

move caching to entity layer

6acb751

++

682e87c

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

jonahwilliams added 4 commits August 14, 2023 19:15

++

ce50348

++

06377e7

++

7302822

++

cf3453f

jonahwilliams removed the Work in progress (WIP) Not ready (yet) for review! label Aug 15, 2023

jonahwilliams requested review from bdero and dnfield August 15, 2023 17:41

bdero approved these changes Aug 15, 2023

View reviewed changes

Jonah Williams and others added 2 commits August 15, 2023 13:52

Update impeller/entity/render_target_cache_unittests.cc

ddd2663

Co-authored-by: Brandon DeRosier <x@bdero.me>

Update texture_descriptor.h

dfbb805

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 15, 2023

auto-submit bot merged commit 659cdfc into flutter:main Aug 16, 2023

jonahwilliams deleted the less_hacky_but_still_hacky_cache branch August 16, 2023 00:36

engine-flutter-autoroll mentioned this pull request Aug 16, 2023

Roll Flutter Engine from 7409ce4ba0a8 to 659cdfc5a568 (1 revision) flutter/flutter#132612

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 16, 2023

659cdfc5a [Impeller] Cache render target texture allocations across f…

c745735

…rames. (flutter/engine#44527)

gaaclarke mentioned this pull request Aug 24, 2023

[Impeller] Refactor: Create attachment descriptions without setting layouts #45088

Merged

8 tasks

jonahwilliams mentioned this pull request Aug 25, 2023

[Impeller] Reduce render-pass overhead when using wide gamut (iOS) flutter/flutter#131567

Closed

		data_to_recycle_.clear();
		data_to_recycle_.insert(data_to_recycle_.end(), retain.begin(), retain.end());

	data_to_recycle_.clear();
	data_to_recycle_.insert(data_to_recycle_.end(), retain.begin(), retain.end());
	data_to_recycle_.swap(retain);

[Impeller] Cache render target texture allocations across frames. #44527

[Impeller] Cache render target texture allocations across frames. #44527

Uh oh!

Conversation

jonahwilliams commented Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

bdero Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Aug 15, 2023

Uh oh!

jonahwilliams commented Aug 15, 2023

Uh oh!

dnfield commented Aug 15, 2023

Uh oh!

jonahwilliams commented Aug 15, 2023

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonahwilliams commented Aug 9, 2023 •

edited

Loading

bdero Aug 14, 2023 •

edited

Loading