[Impeller] avoid heap allocation in RenderTarget object. #56829

jonahwilliams · 2024-11-27T01:17:35Z

All render targets created by flutter have a single color attachment. Lets specialize that case in impeller::RenderTarget by creating a field for that attachment description instead of placing it in a hashmap.

jonahwilliams · 2024-11-27T01:18:10Z

impeller/renderer/render_target.cc

    return false;
  }

+#ifndef NDEBUG


This function is actually crazy expensive and I don't think we should be running it in any modes.

…nto render_pass_stuff

gaaclarke

Approach is looking good to me. I'd rather see this detail abstracted away from RenderTarget's clients instead of changing its interface.

gaaclarke · 2024-12-02T23:48:00Z

impeller/renderer/backend/vulkan/render_pass_builder_vk.cc

+    vk::AttachmentReference color_ref;
+    color_ref.attachment = attachments.size();
+    color_ref.layout = vk::ImageLayout::eGeneral;
+    color_refs[0] = color_ref;
+    attachments.push_back(color0_.value());
+
+    if (color0_resolve_.has_value()) {
+      vk::AttachmentReference resolve_ref;
+      resolve_ref.attachment = attachments.size();
+      resolve_ref.layout = vk::ImageLayout::eGeneral;
+      resolve_refs[0] = resolve_ref;
+      attachments.push_back(color0_resolve_.value());
+    }


Please remove duplication

Can't you use the Iterate method here to avoid the duplication?

This isn't the render_target, this is the internal implementation of the RenderPassBuilderVK

gaaclarke · 2024-12-02T23:48:48Z

impeller/renderer/backend/vulkan/render_pass_builder_vk.h

+  std::optional<vk::AttachmentDescription> color0_resolve_;
+  std::optional<vk::AttachmentDescription> depth_stencil_;
+
  std::map<size_t, vk::AttachmentDescription> colors_;


We should add a comment here that colors_[0] is really at color0_.

gaaclarke · 2024-12-02T23:50:47Z

impeller/entity/entity_pass_target_unittests.cc

-                    .GetColorAttachments()
-                    .find(0u)


Can we instead change this to .GetColorAttachment(0u). That way the way the render target is storing the first color attachment isn't being leaked to its clients.

gaaclarke · 2024-12-02T23:52:51Z

impeller/renderer/render_target.cc

+  return ColorAttachment{};
+}
+
+bool RenderTarget::HasColor0() const {


Same thing here. Let's just make HasColorAttachment(0) look into this.

jonahwilliams · 2024-12-02T23:54:28Z

Approach is looking good to me. I'd rather see this detail abstracted away from RenderTarget's clients instead of changing its interface.

So the way to do that would be to remove the GetColorAttachments function that returns a map and instead have an iterator like approach. I can do that.

gaaclarke · 2024-12-02T23:56:50Z

Approach is looking good to me. I'd rather see this detail abstracted away from RenderTarget's clients instead of changing its interface.

So the way to do that would be to remove the GetColorAttachments function that returns a map and instead have an iterator like approach. I can do that.

I don't think you need need an iterator, deleting GetColorAttachments() and adding GetColorAttachment(size_t) should be enough right?

jonahwilliams · 2024-12-02T23:59:07Z

I don't need an iterator exactly, I just can't have a method that returns a app. I can't just have a length query because I think its possible to have combinations like color0 and color2 with color1 not set to anything.

jonahwilliams · 2024-12-03T01:10:02Z

app -> map

jonahwilliams · 2024-12-03T01:38:33Z

Updated so that we use a new IterateColorAttachments function to avoid making callers handle color0 as a special case.

gaaclarke

LGTM with a couple of remaining niggles. I think this is a lot cleaner.

gaaclarke · 2024-12-03T16:51:38Z

impeller/renderer/backend/vulkan/render_pass_builder_vk.cc

+    vk::AttachmentReference color_ref;
+    color_ref.attachment = attachments.size();
+    color_ref.layout = vk::ImageLayout::eGeneral;
+    color_refs[0] = color_ref;
+    attachments.push_back(color0_.value());
+
+    if (color0_resolve_.has_value()) {
+      vk::AttachmentReference resolve_ref;
+      resolve_ref.attachment = attachments.size();
+      resolve_ref.layout = vk::ImageLayout::eGeneral;
+      resolve_refs[0] = resolve_ref;
+      attachments.push_back(color0_resolve_.value());
+    }


Can't you use the Iterate method here to avoid the duplication?

gaaclarke · 2024-12-03T16:53:42Z

impeller/renderer/backend/vulkan/render_pass_builder_vk.h

+  // Visible for testing.
+  std::optional<vk::AttachmentDescription> GetColor0() const;
+
+  // Visible for testing.
+  std::optional<vk::AttachmentDescription> GetColor0Resolve() const;


Please make the test a friend instead of making these public (example

engine/impeller/renderer/backend/gles/buffer_bindings_gles.h

Line 51 in c7f553e

FML_FRIEND_TEST(testing::BufferBindingsGLESTest, BindUniformData);

)

Listing out each test name seems like a pretty big step down, given that this is an internal library of an internal library.

…lutter/engine#56829)

…159805) flutter/engine@29d6640...1f7f37e 2024-12-04 [email protected] [Impeller] avoid heap allocation in RenderTarget object. (flutter/engine#56829) 2024-12-04 [email protected] Manual roll dart to 470117150f34d712ee6d8c4558b3c656d973f656 (flutter/engine#56915) 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 [email protected],[email protected] 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

…ine#56829) All render targets created by flutter have a single color attachment. Lets specialize that case in impeller::RenderTarget by creating a field for that attachment description instead of placing it in a hashmap.

[Impeller] avoid heap allocation in RenderTarget object.

8e740b5

github-actions bot added the e: impeller label Nov 27, 2024

jonahwilliams commented Nov 27, 2024

View reviewed changes

jonahwilliams added 7 commits November 26, 2024 17:46

fix missing color attachments in vk render_pass.

5b4d07e

Update render_target.cc

24450c4

lint workarounds.

61c4ff5

Update render_target.cc

b6de222

++

c65046a

Merge branch 'render_pass_stuff' of github.com:jonahwilliams/engine i…

938b279

…nto render_pass_stuff

fixup flutter gpu.

132748a

jonahwilliams requested a review from gaaclarke December 2, 2024 20:09

gaaclarke reviewed Dec 2, 2024

View reviewed changes

jonahwilliams added 2 commits December 2, 2024 17:34

hide specialization of color0 attachment.

2f912f6

add comment.

52d2674

jonahwilliams requested a review from gaaclarke December 3, 2024 01:38

gaaclarke approved these changes Dec 3, 2024

View reviewed changes

++

2190282

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

auto-submit bot merged commit 1f7f37e into flutter:main Dec 4, 2024
31 checks passed

engine-flutter-autoroll mentioned this pull request Dec 4, 2024

Roll Flutter Engine from 29d66405e6df to 1f7f37e264b0 (2 revisions) flutter/flutter#159805

Merged

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

1f7f37e26 [Impeller] avoid heap allocation in RenderTarget object. (f…

02e2882

…lutter/engine#56829)

[Impeller] avoid heap allocation in RenderTarget object. #56829

[Impeller] avoid heap allocation in RenderTarget object. #56829

Uh oh!

Conversation

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

Uh oh!

Uh oh!

jonahwilliams Nov 27, 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 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!

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!

jonahwilliams commented Dec 2, 2024

Uh oh!

gaaclarke commented Dec 2, 2024

Uh oh!

jonahwilliams commented Dec 2, 2024

Uh oh!

jonahwilliams commented Dec 3, 2024

Uh oh!

jonahwilliams commented Dec 3, 2024

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!

Uh oh!

Uh oh!

jonahwilliams commented Nov 27, 2024 •

edited

Loading

jonahwilliams Nov 27, 2024 •

edited

Loading