[Impeller] Allocate exact descriptor count, populate in one go. #47200

jonahwilliams · 2023-10-22T04:01:29Z

Rather than doing a guess and check, since we have all of our cmds already stored we can add up the binding counts and allocate the exact descriptor size and populate them in one call.

Also makes render_pass and compute_pass share more (though not all) code.

jonahwilliams · 2023-10-22T17:44:20Z

I did this work as part of an investigation into whether or not we could use dynamic uniform buffers. What I found is that would be difficult as the dynamic descriptor API requires the uniform buffers to have equal sizing - meaning we'd need to adjust our headers and maintain some additional tooling to synchronize that. Given that descriptors updates dont consitute a huge overhead today, I settled for a refactor to clarify the code, remove extra allocations, and remove duplication across compute_pass and render_pass

…engine into descriptor_simplification

jonahwilliams · 2023-10-23T00:13:48Z

impeller/renderer/backend/vulkan/binding_helpers.cc

+    if (!encoder->Track(texture) ||
+        !encoder->Track(sampler.GetSharedSampler())) {
+      return false;
+    }


TODO: we're currently calling track twice on each texture.

FWIW, tracking the same resource multiple times is safe.

chinmaygarde

Other than the confusion about error handling in AllocateDescriptorSets, lgtm.

chinmaygarde · 2023-10-23T21:30:49Z

impeller/renderer/backend/vulkan/BUILD.gn

    "android_hardware_buffer_texture_source_vk.h",
    "barrier_vk.cc",
    "barrier_vk.h",
+    "binding_helpers.cc",


Nit: binding_helpers_vk?

chinmaygarde · 2023-10-23T21:36:08Z

impeller/renderer/backend/vulkan/binding_helpers.h

+    const std::shared_ptr<CommandEncoderVK>& encoder,
+    const std::vector<Command>& commands);
+
+std::vector<vk::DescriptorSet> AllocateAndBindDescriptorSets(


Ooh, the overload is unfortunate. Trying to figure out how we can dry up this code. But perhaps I am overthinking it.

I could make this a template monstrosity but I think lets take the win for now from sharing BindTexture/BindBuffer. Long term, I think we fix this by breaking apart Command/ComputeCommand and storing the encoding data more generically

Yeah, I was going to suggest reworking it but realized thats going to be non-trivial. Later, if at all.

chinmaygarde · 2023-10-23T21:36:41Z

impeller/renderer/backend/vulkan/binding_helpers.cc

+    if (!encoder->Track(texture) ||
+        !encoder->Track(sampler.GetSharedSampler())) {
+      return false;
+    }


FWIW, tracking the same resource multiple times is safe.

chinmaygarde · 2023-10-23T21:40:44Z

impeller/renderer/backend/vulkan/descriptor_pool_vk.cc

-    size_t command_count) {
-  if (pools_.empty()) {
-    pool_size_ = command_count;
+std::vector<vk::DescriptorSet> DescriptorPoolVK::AllocateDescriptorSets(


This weakens the error reporting ability of this API though. An empty vector isn't the same as a vector with nullopt elements.

Yeah, I think this should be something like a status_or

I updated these to status_or. not totally in love with the existing status enums, and the messages feel a bit redundant witht the validation logs.

chinmaygarde · 2023-10-23T21:41:30Z

impeller/renderer/backend/vulkan/descriptor_pool_vk.h

-  std::optional<vk::DescriptorSet> AllocateDescriptorSet(
-      const vk::DescriptorSetLayout& layout,
-      size_t command_count);
+  std::vector<vk::DescriptorSet> AllocateDescriptorSets(


Lets document what an empty vector would mean. Ideally, this would be a vector of optional elements.

What would a std::nullopt descriptor set imply?

Nevermind. I thought it was a DescriptorSetUnique. Following this up by making it a status or (or just leave it be) is fine.

chinmaygarde · 2023-10-23T21:41:59Z

impeller/renderer/backend/vulkan/descriptor_pool_vk.cc

+  auto new_pool =
+      CreatePool(strong_device->GetDevice(), sampler_count, buffer_count);
+  if (!new_pool) {
+    return {};


Maybe a validation log here saying the pool could not be allocated.

… go. (flutter/engine#47200)

…137180) flutter/engine@602b513...d6a48e9 2023-10-24 [email protected] [Impeller] Allocate exact descriptor count, populate in one go. (flutter/engine#47200) 2023-10-24 [email protected] [Impeller] Curve components in stroke path use start directions as their initial offsets (flutter/engine#46203) 2023-10-24 [email protected] Roll Fuchsia Mac SDK from OUWiWfUxyMyDOlEfA... to YqSO1OByhoexFJSCr... (flutter/engine#47273) 2023-10-24 [email protected] [Impeller] Enable MSAA for OpenGLES: Take 2. (flutter/engine#47030) 2023-10-24 [email protected] Manual roll Dart SDK from 901e92d10627 to 360370ff93b0 (3 revisions) (flutter/engine#47271) 2023-10-24 [email protected] Roll Dart SDK from 901e92d10627 to 360370ff93b0 (3 revisions) (flutter/engine#47269) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from OUWiWfUxyMyD to YqSO1OByhoex 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],[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

[Impeller] allocate exact descriptor count, populate in one go.

6129a5c

github-actions bot added the e: impeller label Oct 22, 2023

++

697093f

Jonah Williams added 3 commits October 22, 2023 10:44

Merge branch 'main' into descriptor_simplification

fa38735

++

7998ded

Merge branch 'descriptor_simplification' of github.com:jonahwilliams/…

49c8a8e

…engine into descriptor_simplification

jonahwilliams commented Oct 23, 2023

View reviewed changes

chinmaygarde changed the title ~~[Impeller] allocate exact descriptor count, populate in one go.~~ [Impeller] Allocate exact descriptor count, populate in one go. Oct 23, 2023

chinmaygarde assigned jonahwilliams Oct 23, 2023

jonahwilliams marked this pull request as ready for review October 23, 2023 18:05

chinmaygarde approved these changes Oct 23, 2023

View reviewed changes

jonahwilliams added 5 commits October 23, 2023 16:56

code review

dc78d28

++

1e61f01

++

9150b26

++

cf07f32

++

12f37db

jonahwilliams mentioned this pull request Oct 24, 2023

[Impeller] re-use descriptor for uniform buffer bindings in Vulkan render pass encoder. flutter/flutter#132589

Closed

chinmaygarde approved these changes Oct 24, 2023

View reviewed changes

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

auto-submit bot merged commit d6a48e9 into flutter:main Oct 24, 2023

engine-flutter-autoroll mentioned this pull request Oct 24, 2023

Roll Flutter Engine from 602b5131ee4d to d6a48e9963df (6 revisions) flutter/flutter#137180

Merged

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

d6a48e996 [Impeller] Allocate exact descriptor count, populate in one…

58a4f59

… go. (flutter/engine#47200)

[Impeller] Allocate exact descriptor count, populate in one go. #47200

[Impeller] Allocate exact descriptor count, populate in one go. #47200

Uh oh!

Conversation

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

Uh oh!

Uh oh!

jonahwilliams commented Oct 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde 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!

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!

jonahwilliams commented Oct 22, 2023 •

edited

Loading