[Impeller] Check for lazy memory support. #43339

jonahwilliams · 2023-06-29T18:58:49Z

Some Android devices do not support the memory type eLazilyAllocated, which we use for MSAA and stencil textures. These textures were falling back to device local in dedicated allocations, which are expensive to both allocate and free. The dedicated allocation is implied by asking for eLazilyAllocated

Instead, perform a check for support for this memory type. Never request dedicated allocations (at least not until we have a compelling use case)

This should fix flutter/flutter#129737

flutter/flutter#129784

flutter-dashboard · 2023-06-29T18:58:52Z

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.

jonahwilliams · 2023-06-29T18:59:20Z

impeller/renderer/backend/vulkan/allocator_vk.cc

      }
      return flags;
    case StorageMode::kDevicePrivate:
-      if (is_texture) {


Lets not use VMA_ALLOCATION_CREATE_DEDICATED_MEMORY_BIT until we have a case where it performs better

I thought Angle did that for > 4MB allocations though. Perhaps just start off with that assumption, but for non-render targets?

Lets follow up on this one in #43325 ?

If we don't remove this bit we'll have the same performance issues as dedicated allocations don't go into the pool.

Sounds good.

jonahwilliams · 2023-06-29T18:59:38Z

impeller/renderer/backend/vulkan/allocator_vk.h

  std::weak_ptr<DeviceHolder> device_holder_;
  ISize max_texture_size_;
  bool is_valid_ = false;
+  bool supports_lazy_memory_ = false;


I didn't put this in caps since its not clear that anything outside of the allocator needs to know yet...

Perhaps call this supports_memoryless_textures_ since we have used the term before and are familiar with it?

chinmaygarde · 2023-06-29T19:07:23Z

impeller/renderer/backend/vulkan/allocator_vk.h

  std::weak_ptr<DeviceHolder> device_holder_;
  ISize max_texture_size_;
  bool is_valid_ = false;
+  bool supports_lazy_memory_ = false;


Perhaps call this supports_memoryless_textures_ since we have used the term before and are familiar with it?

chinmaygarde · 2023-06-29T19:09:12Z

impeller/renderer/backend/vulkan/allocator_vk.cc

  return max_texture_size_;
 }

+void AllocatorVK::CheckForMemoryTypeSupport() {


IMO, this should just be a cap check set in CapabilitiesVK::SetPhysicalDevice. That way, if we decide this is a dealbreaker for Vulkan, we can decide to more easily terminate context setup and fallback to OpenGL. The check here is too late to make that call.

chinmaygarde · 2023-06-29T19:10:42Z

impeller/renderer/backend/vulkan/allocator_vk.cc

  VmaAllocationCreateInfo allocation_info = {};
  allocation_info.usage = ToVMAMemoryUsage();
-  allocation_info.preferredFlags = ToVKMemoryPropertyFlags(desc.storage_mode);
+  allocation_info.preferredFlags =


We should probably use required instead of preferred flags now since we are sure the device supports the type.

Ditto for the texture allocation too.

Yeah can do that. We will also need to cap check for host visible textures as well.

I wasn't able to get this working with some quick effort. lets track switching to required flags once we've chased down all the loose ends.

chinmaygarde · 2023-06-29T19:12:24Z

impeller/renderer/backend/vulkan/allocator_vk.cc

      return VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT;
    case StorageMode::kDeviceTransient:
-      return VK_MEMORY_PROPERTY_LAZILY_ALLOCATED_BIT;
+      if (supports_lazy_memory) {


Looking at the memory types on vulkan.gpuinfo.org. We should be &ing these to be both device local and lazily allocated (if supported).

chinmaygarde · 2023-06-29T19:46:06Z

I didn't put this in caps since its not clear that anything outside of the allocator needs to know yet...

I was putting stuff in caps not because many components would check it. But because we could terminate context setup and fallback to GL at any time based on the presence of a specific cap.

jonahwilliams · 2023-06-29T20:40:40Z

I haven't been able to get the right check for host visible textures working, so I'm leaving us with preferred flags for now.

chinmaygarde

Some nits (like the cap check), but otherwise LGTM. I'll add a followup for what to do about devices in the lab that have device transient texture support.

jonahwilliams · 2023-06-29T20:51:50Z

I'm less optimistic about the improvements locally. Will study ANGLE more.

…3339)

…129818) flutter/engine@d333434...099a70e 2023-06-30 ditman@gmail.com [web] Add nonce configuration. (flutter/engine#42829) 2023-06-30 bdero@google.com [Impeller] Unwrap optional procs in EntityPass (flutter/engine#43352) 2023-06-30 bdero@google.com [Impeller] Assign missing user to TODO. (flutter/engine#43351) 2023-06-29 skia-flutter-autoroll@skia.org Roll Skia from a1ae27969207 to 2d05e3ec6b67 (1 revision) (flutter/engine#43350) 2023-06-29 skia-flutter-autoroll@skia.org Roll Dart SDK from ecc2440be198 to 2d98d9e27dae (1 revision) (flutter/engine#43347) 2023-06-29 jonahwilliams@google.com [Impeller] Check for lazy memory support. (flutter/engine#43339) 2023-06-29 bdero@google.com [Impeller] Remove all double empties (flutter/engine#43345) 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 jimgraham@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

Some Android devices do not support the memory type eLazilyAllocated, which we use for MSAA and stencil textures. These textures were falling back to device local in dedicated allocations, which are expensive to both allocate and free. The dedicated allocation is implied by asking for eLazilyAllocated Instead, perform a check for support for this memory type. Never request dedicated allocations (at least not until we have a compelling use case) This should fix flutter/flutter#129737 flutter/flutter#129784

jonahwilliams added 2 commits June 29, 2023 11:55

[Impeller] explicitly check for LAZILY_ALLOCATED memory support.

a94e409

++

48f3e9d

flutter-dashboard bot added the needs tests label Jun 29, 2023

github-actions bot added the e: impeller label Jun 29, 2023

jonahwilliams commented Jun 29, 2023

View reviewed changes

jonahwilliams requested a review from chinmaygarde June 29, 2023 18:59

chinmaygarde reviewed Jun 29, 2023

View reviewed changes

chinmaygarde approved these changes Jun 29, 2023

View reviewed changes

cap check

bb99f08

++

bea4104

jonahwilliams removed the needs tests label Jun 29, 2023

dont put transient attachment on image.

5dbac54

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

auto-submit bot merged commit 12f3f47 into flutter:main Jun 29, 2023

jonahwilliams deleted the check_for_lazy_memory_support branch June 29, 2023 22:39

engine-flutter-autoroll mentioned this pull request Jun 30, 2023

Roll Flutter Engine from d33343430f18 to 099a70ebbc60 (7 revisions) flutter/flutter#129818

Merged

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

12f3f47db [Impeller] Check for lazy memory support. (flutter/engine#4…

ae3b3fe

…3339)

[Impeller] Check for lazy memory support. #43339

[Impeller] Check for lazy memory support. #43339

Uh oh!

Conversation

jonahwilliams commented Jun 29, 2023

Uh oh!

flutter-dashboard bot commented Jun 29, 2023

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!

chinmaygarde commented Jun 29, 2023

Uh oh!

jonahwilliams commented Jun 29, 2023

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants