[Impeller] Cache RenderPass/Framebuffer objects on the resolve texture sources. #50142

jonahwilliams · 2024-01-29T17:44:47Z

Cache vk::RenderPass and vk::Framebuffer objects on the resolve texture of any render target attachments. Use these on the next frame unconditionally.

Fixes flutter/flutter#141750

flutter-dashboard · 2024-02-01T22:05:48Z

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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 · 2024-02-01T22:11:59Z

I guess this isn't directly tested but a test did fail until I fixed it 🤔

impeller/renderer/backend/vulkan/render_pass_vk.cc

impeller/renderer/backend/vulkan/texture_vk.h

gaaclarke · 2024-02-01T23:01:54Z

impeller/renderer/backend/vulkan/texture_vk.h

@@ -44,9 +44,20 @@ class TextureVK final : public Texture, public BackendCast<TextureVK, Texture> {

  bool IsSwapchainImage() const { return source_->IsSwapchainImage(); }

+  // These methods should only be used by render_pass_vk.h
+  void SetFramebuffer(const SharedHandleVK<vk::Framebuffer>& framebuffer);


Would it make sense to save this on the RenderTarget, not the Texture? This isn't applicable to every Texture, right?

The RenderTarget itself is const/immutable whereas the Texture is stateful.

That's not a problem though. We can make the frame buffer and render pass when we create the RenderTarget. We shouldn't ever have a RenderTarget without a render pass and it will never change, right?

Unfortunately that is all constructed above the backend layer, and there are no metal or gles analogs for these objects.

impeller/renderer/backend/vulkan/render_pass_vk.cc

gaaclarke

This looks good, I just have a style nit and the obligatory question on testing. Great job finding a place to cache these relatively easily. I would have preferred this lives on the RenderTarget but I understand that would be a pretty invasive change.

You should be able to add a test for this in AiksTests. There is an example in GaussianBlurMipMapSolidColor where we render something and check the texture cache.

I was thinking you could render once, then inspect the texture cache and if it's vulkan make sure the GetRenderPass and GetFramePass are non-null for a texture that is IsSwapchainImage. Something like that.

You could then render a second frame and make sure that texture cache size is still 1. So we could presume that it was reused. We can't assert it however. That would make sure it doesn't crash and generates output.

Alternatively, or in addition, you could make a test directly on render_pass_vk.cc. This has a higher upfront cost but is easier to maintain and can be more precise than AiksTests.

Right now, someone could just comment out the call to SetRenderPass and no tests would fail. Presumably the benchmarks would catch that, but catching things in benchmarks is more expensive and less trustworthy. We may not even have an automated test that checks that a second render will not crash. I have one blur test that does that (GuassianBlurUpdatesMipmapContents). Maybe we'd benefit from something more apropos though.

gaaclarke · 2024-02-02T19:34:59Z

impeller/renderer/backend/vulkan/texture_vk.h

+  /// May be nullptr if no previous framebuffer existed.
+  SharedHandleVK<vk::Framebuffer> GetFramebuffer() const;
+
+  /// @brief Retrieve the last render pass object used with this texture.


fyi: @brief is redundant here since the first section is implicitly the brief section.

gaaclarke · 2024-02-02T19:48:15Z

impeller/renderer/backend/vulkan/texture_vk.cc

+}
+
+SharedHandleVK<vk::RenderPass> TextureVK::GetRenderPass() const {
+  return renderpass_;


These are inconsistent about wether render pass is 2 words or one. It should be RenderPass and render_pass_ or Renderpass and renderpass_.

Same for "frame buffer".

Yeah, render pass is two words, framebuffer is one word.

gaaclarke · 2024-02-02T19:48:49Z

impeller/renderer/backend/vulkan/texture_vk.h

@@ -44,9 +44,37 @@ class TextureVK final : public Texture, public BackendCast<TextureVK, Texture> {

  bool IsSwapchainImage() const { return source_->IsSwapchainImage(); }

+  // These methods should only be used by render_pass_vk.h


Comments look good 👍

jonahwilliams · 2024-02-02T20:44:47Z

Unfortunately I don't think we can import the Vulkan types into any of the aiks tests. 🤔

jonahwilliams · 2024-02-02T20:45:50Z

best bet is probably a vulkan unit test that creates real objects for the most part. Should be doable via playground. I'll give that a shot.

jonahwilliams · 2024-02-02T21:32:00Z

impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc

+  auto resolve_texture =
+      render_target.GetColorAttachments().find(0u)->second.resolve_texture;
+  auto& texture_vk = TextureVK::Cast(*resolve_texture);
+


This test appears to run with validations locally. There were no validation errors I believe

gaaclarke

LGTM! Exciting change, thanks.

…lve texture sources. (flutter/engine#50142)

…143005) flutter/engine@8088863...07cdaab 2024-02-06 skia-flutter-autoroll@skia.org Roll Dart SDK from 29265c94a6e8 to 35269fa71956 (1 revision) (flutter/engine#50402) 2024-02-06 54558023+keyonghan@users.noreply.github.com Add use_rbe to gclient variables for Framework Smoke Tests (flutter/engine#50403) 2024-02-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Revert "Revert "[Fuchsia] Execute most of the testing/fuchsia/test_suites.yaml on debug and release builds""" (flutter/engine#50407) 2024-02-06 matanlurey@users.noreply.github.com Skip flaking test on Windows nobody is fixing. (flutter/engine#50401) 2024-02-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[web] Fix Scene clip bounds. Trigger resize on DPR Change." (flutter/engine#50404) 2024-02-06 6844906+zijiehe-google-com@users.noreply.github.com Revert "Revert "[Fuchsia] Execute most of the testing/fuchsia/test_suites.yaml on debug and release builds"" (flutter/engine#50295) 2024-02-06 chinmaygarde@google.com [Impeller] Specify if Angle or SwiftShader is being used in the title. (flutter/engine#50376) 2024-02-06 matanlurey@users.noreply.github.com Run all Android `scenario_app` tests, not just the smoke test. (flutter/engine#50400) 2024-02-06 skia-flutter-autoroll@skia.org Roll Dart SDK from b62066b42af0 to 29265c94a6e8 (10 revisions) (flutter/engine#50398) 2024-02-06 matanlurey@users.noreply.github.com Capture `FAILURES!!!` when running Android `scenario_app` tests. (flutter/engine#50255) 2024-02-06 jonnywang@google.com [fuchsia] Bump Fuchsia's API level to 16 (flutter/engine#50358) 2024-02-06 skia-flutter-autoroll@skia.org Roll Skia from 9e68ed6caf6d to 44106ee8edea (1 revision) (flutter/engine#50393) 2024-02-06 skia-flutter-autoroll@skia.org Roll Skia from c29a20702356 to 9e68ed6caf6d (1 revision) (flutter/engine#50392) 2024-02-06 jonahwilliams@google.com [Impeller] Cache RenderPass/Framebuffer objects on the resolve texture sources. (flutter/engine#50142) 2024-02-06 jason-simmons@users.noreply.github.com [Impeller] Do not skip the GLES render pass if the command list is empty (flutter/engine#50381) 2024-02-06 skia-flutter-autoroll@skia.org Roll Skia from cdf214adfb4d to c29a20702356 (54 revisions) (flutter/engine#50382) 2024-02-06 ditman@gmail.com [web] Fix Scene clip bounds. Trigger resize on DPR Change. (flutter/engine#50161) 2024-02-06 xilaizhang@google.com [github actions] add cherry pick workflow for engine repo (flutter/engine#50265) 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 chinmaygarde@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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…visions)" (#143025) Reverts #143005 Initiated by: vashworth Reason for reverting: `Linux technical_debt` started failing on this commit: https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20technical_debt__cost/16684/overview Original PR Author: engine-flutter-autoroll Reviewed By: {fluttergithubbot} This change reverts the following previous change: Original Description: flutter/engine@8088863...07cdaab 2024-02-06 skia-flutter-autoroll@skia.org Roll Dart SDK from 29265c94a6e8 to 35269fa71956 (1 revision) (flutter/engine#50402) 2024-02-06 54558023+keyonghan@users.noreply.github.com Add use_rbe to gclient variables for Framework Smoke Tests (flutter/engine#50403) 2024-02-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Revert "Revert "[Fuchsia] Execute most of the testing/fuchsia/test_suites.yaml on debug and release builds""" (flutter/engine#50407) 2024-02-06 matanlurey@users.noreply.github.com Skip flaking test on Windows nobody is fixing. (flutter/engine#50401) 2024-02-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[web] Fix Scene clip bounds. Trigger resize on DPR Change." (flutter/engine#50404) 2024-02-06 6844906+zijiehe-google-com@users.noreply.github.com Revert "Revert "[Fuchsia] Execute most of the testing/fuchsia/test_suites.yaml on debug and release builds"" (flutter/engine#50295) 2024-02-06 chinmaygarde@google.com [Impeller] Specify if Angle or SwiftShader is being used in the title. (flutter/engine#50376) 2024-02-06 matanlurey@users.noreply.github.com Run all Android `scenario_app` tests, not just the smoke test. (flutter/engine#50400) 2024-02-06 skia-flutter-autoroll@skia.org Roll Dart SDK from b62066b42af0 to 29265c94a6e8 (10 revisions) (flutter/engine#50398) 2024-02-06 matanlurey@users.noreply.github.com Capture `FAILURES!!!` when running Android `scenario_app` tests. (flutter/engine#50255) 2024-02-06 jonnywang@google.com [fuchsia] Bump Fuchsia's API level to 16 (flutter/engine#50358) 2024-02-06 skia-flutter-autoroll@skia.org Roll Skia from 9e68ed6caf6d to 44106ee8edea (1 revision) (flutter/engine#50393) 2024-02-06 skia-flutter-autoroll@skia.org Roll Skia from c29a20702356 to 9e68ed6caf6d (1 revision) (flutter/engine#50392) 2024-02-06 jonahwilliams@google.com [Impeller] Cache RenderPass/Framebuffer objects on the resolve texture sources. (flutter/engine#50142) 2024-02-06 jason-simmons@users.noreply.github.com [Impeller] Do not skip the GLES render pass if the command list is empty (flutter/engine#50381) 2024-02-06 skia-flutter-autoroll@skia.org Roll Skia from cdf214adfb4d to c29a20702356 (54 revisions) (flutter/engine#50382) 2024-02-06 ditman@gmail.com [web] Fix Scene clip bounds. Trigger resize on DPR Change. (flutter/engine#50161) 2024-02-06 xilaizhang@google.com [github actions] add cherry pick workflow for engine repo (flutter/engine#50265) 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 chinmaygarde@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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lve texture sources. (flutter/engine#50142)

…sions) (#143039) Manual roll requested by zra@google.com flutter/engine@8088863...1ac6beb 2024-02-07 skia-flutter-autoroll@skia.org Roll Skia from fd55a6bd3580 to 1b266b36a4c8 (1 revision) (flutter/engine#50423) 2024-02-06 skia-flutter-autoroll@skia.org Roll Skia from 44106ee8edea to fd55a6bd3580 (17 revisions) (flutter/engine#50420) 2024-02-06 bdero@google.com [Impeller] Fix pipeline attachment layout in CanRenderToTexture. (flutter/engine#50413) 2024-02-06 ian@hixie.ch Provide toStrings for Native objects (flutter/engine#50168) 2024-02-06 skia-flutter-autoroll@skia.org Roll Dart SDK from 29265c94a6e8 to 35269fa71956 (1 revision) (flutter/engine#50402) 2024-02-06 54558023+keyonghan@users.noreply.github.com Add use_rbe to gclient variables for Framework Smoke Tests (flutter/engine#50403) 2024-02-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Revert "Revert "[Fuchsia] Execute most of the testing/fuchsia/test_suites.yaml on debug and release builds""" (flutter/engine#50407) 2024-02-06 matanlurey@users.noreply.github.com Skip flaking test on Windows nobody is fixing. (flutter/engine#50401) 2024-02-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[web] Fix Scene clip bounds. Trigger resize on DPR Change." (flutter/engine#50404) 2024-02-06 6844906+zijiehe-google-com@users.noreply.github.com Revert "Revert "[Fuchsia] Execute most of the testing/fuchsia/test_suites.yaml on debug and release builds"" (flutter/engine#50295) 2024-02-06 chinmaygarde@google.com [Impeller] Specify if Angle or SwiftShader is being used in the title. (flutter/engine#50376) 2024-02-06 matanlurey@users.noreply.github.com Run all Android `scenario_app` tests, not just the smoke test. (flutter/engine#50400) 2024-02-06 skia-flutter-autoroll@skia.org Roll Dart SDK from b62066b42af0 to 29265c94a6e8 (10 revisions) (flutter/engine#50398) 2024-02-06 matanlurey@users.noreply.github.com Capture `FAILURES!!!` when running Android `scenario_app` tests. (flutter/engine#50255) 2024-02-06 jonnywang@google.com [fuchsia] Bump Fuchsia's API level to 16 (flutter/engine#50358) 2024-02-06 skia-flutter-autoroll@skia.org Roll Skia from 9e68ed6caf6d to 44106ee8edea (1 revision) (flutter/engine#50393) 2024-02-06 skia-flutter-autoroll@skia.org Roll Skia from c29a20702356 to 9e68ed6caf6d (1 revision) (flutter/engine#50392) 2024-02-06 jonahwilliams@google.com [Impeller] Cache RenderPass/Framebuffer objects on the resolve texture sources. (flutter/engine#50142) 2024-02-06 jason-simmons@users.noreply.github.com [Impeller] Do not skip the GLES render pass if the command list is empty (flutter/engine#50381) 2024-02-06 skia-flutter-autoroll@skia.org Roll Skia from cdf214adfb4d to c29a20702356 (54 revisions) (flutter/engine#50382) 2024-02-06 ditman@gmail.com [web] Fix Scene clip bounds. Trigger resize on DPR Change. (flutter/engine#50161) 2024-02-06 xilaizhang@google.com [github actions] add cherry pick workflow for engine repo (flutter/engine#50265) 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 chinmaygarde@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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

zanderso · 2024-02-08T16:12:05Z

Guessing this change is responsible for these improvements in SkiaPerf:

https://flutter-flutter-perf.skia.org/e/?begin=1706163037&end=1707406640&keys=X8aace9d99a1bd6677ed3e4735fa8d220&requestType=0&selected=commit%3D39211%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DMoto_G__4_%252Cdevice_version%253Dnone%252Chost_type%253Dlinux%252Csub_result%253D99th_percentile_frame_rasterizer_time_millis%252Ctest%253Dlist_text_layout_impeller_perf__e2e_summary%252C&xbaroffset=39211

gaaclarke · 2024-02-08T16:53:58Z

Did we ever look into 2024-01-30 jonahwilliams@google.com [Impeller] Reland: add interface for submitting multiple command buffers at once. (flutter/engine#50180)? That seemed to regress that benchmark. It looks good now after this.

jonahwilliams · 2024-02-08T16:57:05Z

No, but that is a good point. I'd like the keep the interface there, but I could adjust the batching down to 1 (immediately submit) and see if the numbers improve further or if it does nothing.

[Impeller] hacking caching of onscreen render pass.

6bbae13

github-actions bot added the e: impeller label Jan 29, 2024

jonahwilliams mentioned this pull request Jan 30, 2024

[Impeller] Consider caching or at least trying to reuse VkFramebuffer and RenderPass objects flutter/flutter#141750

Closed

jonahwilliams added 4 commits January 31, 2024 22:29

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

b3f4b88

heck this works.

a292db7

++

ffcefd1

enable validation.

4463ab8

jonahwilliams changed the title ~~[Impeller] hacking caching of onscreen render pass.~~ [Impeller] Cache RenderPass/Framebuffer objects on the resolve texture sources. Feb 1, 2024

jonahwilliams added 2 commits February 1, 2024 10:31

move cache to texture itself.

43dd21c

NPE.

216e185

jonahwilliams marked this pull request as ready for review February 1, 2024 22:05

jonahwilliams requested a review from gaaclarke February 1, 2024 22:07

gaaclarke reviewed Feb 1, 2024

View reviewed changes

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

gaaclarke reviewed Feb 1, 2024

View reviewed changes

aaron review.

6873355

gaaclarke reviewed Feb 2, 2024

View reviewed changes

++

2b7e44f

jonahwilliams commented Feb 2, 2024

View reviewed changes

add exclusion.

c11a0e2

jonahwilliams requested a review from gaaclarke February 2, 2024 22:48

gaaclarke approved these changes Feb 6, 2024

View reviewed changes

Merge branch 'main' into render_pass_cache

089b9ec

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 6, 2024

auto-submit bot merged commit 319ff6c into flutter:main Feb 6, 2024

jonahwilliams deleted the render_pass_cache branch February 6, 2024 04:35

jonahwilliams mentioned this pull request Feb 6, 2024

[Impeller] vulkan: recycle vkRenderPass and vkFramebuffer flutter/flutter#138856

Closed

engine-flutter-autoroll mentioned this pull request Feb 6, 2024

Roll Flutter Engine from 808886312e2b to e116f89ff506 (11 revisions) flutter/flutter#142997

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2024

319ff6c8c [Impeller] Cache RenderPass/Framebuffer objects on the reso…

49a8017

…lve texture sources. (flutter/engine#50142)

engine-flutter-autoroll mentioned this pull request Feb 6, 2024

Roll Flutter Engine from 808886312e2b to 07cdaab7f531 (18 revisions) flutter/flutter#143005

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2024

319ff6c8c [Impeller] Cache RenderPass/Framebuffer objects on the reso…

f26fd6e

…lve texture sources. (flutter/engine#50142)

auto-submit bot mentioned this pull request Feb 6, 2024

Reverts "Roll Flutter Engine from 808886312e2b to 07cdaab7f531 (18 revisions)" flutter/flutter#143025

Merged

engine-flutter-autoroll mentioned this pull request Feb 7, 2024

Roll Flutter Engine from 808886312e2b to 9c67120fa023 (21 revisions) flutter/flutter#143029

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 7, 2024

319ff6c8c [Impeller] Cache RenderPass/Framebuffer objects on the reso…

7b66fe8

…lve texture sources. (flutter/engine#50142)

engine-flutter-autoroll mentioned this pull request Feb 7, 2024

Manual roll Flutter Engine from 808886312e2b to 1ac6beb8a3c2 (22 revisions) flutter/flutter#143039

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 7, 2024

319ff6c8c [Impeller] Cache RenderPass/Framebuffer objects on the reso…

0153a40

…lve texture sources. (flutter/engine#50142)

		@@ -44,9 +44,37 @@ class TextureVK final : public Texture, public BackendCast<TextureVK, Texture> {

		bool IsSwapchainImage() const { return source_->IsSwapchainImage(); }

		// These methods should only be used by render_pass_vk.h

[Impeller] Cache RenderPass/Framebuffer objects on the resolve texture sources. #50142

[Impeller] Cache RenderPass/Framebuffer objects on the resolve texture sources. #50142

Uh oh!

Conversation

jonahwilliams commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Feb 1, 2024

Uh oh!

jonahwilliams commented Feb 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaaclarke Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

gaaclarke Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

gaaclarke Feb 2, 2024 • 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!

jonahwilliams Feb 2, 2024 • 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!

gaaclarke Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Feb 2, 2024

Uh oh!

jonahwilliams commented Feb 2, 2024

Uh oh!

jonahwilliams Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso commented Feb 8, 2024

Uh oh!

gaaclarke commented Feb 8, 2024

Uh oh!

jonahwilliams commented Feb 8, 2024

Uh oh!

Uh oh!

jonahwilliams commented Jan 29, 2024 •

edited

Loading

gaaclarke Feb 2, 2024 •

edited

Loading

jonahwilliams Feb 2, 2024 •

edited

Loading