[Impeller] Wire Flutter's own `VulkanMemoryAllocator` implementation #37018

iskakaushik · 2022-10-25T21:34:54Z

flutter-dashboard · 2022-10-25T21:34:56Z

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.

ci/licenses_golden/licenses_flutter

chinmaygarde

Oof, didn't realize chasing all the uses of Vulkan context creation would be so tedious. Thanks for doing this!

Left some comments on how Impeller ought to use VMA but there are just informational.

chinmaygarde · 2022-10-27T18:27:25Z

flutter_vma/flutter_skia_vma.h

+
+namespace flutter {
+
+class FlutterSkiaVulkanMemoryAllocator : public skgpu::VulkanMemoryAllocator {


Nit: Perhaps just call this SkiaVulkanMemoryAllocator?

chinmaygarde · 2022-10-27T18:27:38Z

flutter_vma/flutter_skia_vma.h

+  // devices are required to support a host visible and coherent memory type.
+  // This is used to work around bugs for devices that don't handle non coherent
+  // memory correctly.
+  bool must_use_coherent_host_visible_memory_;


Disallow copy and assign here?

chinmaygarde · 2022-10-27T18:36:47Z

flutter_vma/flutter_skia_vma.cc

+      // this type of memory. But if we ever do we could pass in an
+      // AllocationPropertyFlag that requests the cached property.
+      info.requiredFlags = VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT |
+                           VK_MEMORY_PROPERTY_HOST_COHERENT_BIT;


In case of Impeller, we should never require VK_MEMORY_PROPERTY_HOST_COHERENT_BIT as we will always have the corresponding map-unmap pairs.

chinmaygarde · 2022-10-27T18:37:25Z

flutter_vma/flutter_skia_vma.cc

+    info.requiredFlags |= VK_MEMORY_PROPERTY_HOST_COHERENT_BIT;
+  }
+  if (kDedicatedAllocation_AllocationPropertyFlag & allocationPropertyFlags) {
+    info.flags |= VMA_ALLOCATION_CREATE_DEDICATED_MEMORY_BIT;


This reminds me, perhaps we should do this for all images from the image decoder (the ui.Images)?

Noted, will fix this in allocator_vk.cc

chinmaygarde · 2022-10-27T18:38:14Z

flutter_vma/flutter_skia_vma.cc

+  }
+  if ((kLazyAllocation_AllocationPropertyFlag & allocationPropertyFlags) &&
+      BufferUsage::kGpuOnly == usage) {
+    info.preferredFlags |= VK_MEMORY_PROPERTY_LAZILY_ALLOCATED_BIT;


In Impellers case, this would be the device transient types.

Noted, will fix this in allocator_vk.cc

chinmaygarde · 2022-10-27T18:38:52Z

flutter_vma/flutter_skia_vma.cc

+
+  if (kPersistentlyMapped_AllocationPropertyFlag & allocationPropertyFlags) {
+    SkASSERT(BufferUsage::kGpuOnly != usage);
+    info.flags |= VMA_ALLOCATION_CREATE_MAPPED_BIT;


We should never needs this with Impeller.

chinmaygarde · 2022-10-27T18:41:00Z

flutter_vma/BUILD.gn

+  ]
+
+  if (disable_vma_stl_shared_mutex) {
+    defines = [ "VMA_USE_STL_SHARED_MUTEX=0" ]


Is this correct? Does this mean there is no thread safety around VMA access? Or does it mean there is a non-STL implementation of mutexes? If there is no thread safety, we'll need to follow up on implementing external synchronization in the Impeller case. Which might be the right thing to do anyway.

This means that they use non-stl mutexes. For context, Impeller doesn't use flutter_skia_vma.cc stuff at all. We just use allocator_vk.cc copy of the vma.

auto-submit · 2022-10-27T19:20:14Z

auto label is removed for flutter/engine, pr: 37018, due to - The status or check suite Mac Host clang-tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.

zanderso · 2022-10-27T22:09:40Z

Is this flake a couple of commits past this change related? https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20Production%20Engine%20Drone/5722/overview

…mentation (flutter/engine#37018)

flutter-dashboard bot added embedder Related to the embedder API needs tests labels Oct 25, 2022

iskakaushik force-pushed the engine_vma branch 3 times, most recently from 030a7ed to 6abf1dd Compare October 26, 2022 14:20

iskakaushik added the e: impeller label Oct 26, 2022

iskakaushik force-pushed the engine_vma branch 2 times, most recently from 10b160b to b80cffe Compare October 26, 2022 16:58

iskakaushik added 14 commits October 27, 2022 11:18

[WIP] Wire Flutter's own VMA

8b8ec43

vma with exts

19312fa

fixup a lot of things

7367377

fix licenses

f2ea939

more fixes

ed0ef62

get things working on linux first

826d390

disable shared mutex on some platforms

10a957e

fix linter issues

73da3f6

Migrate generally useful vulkan procs to their own sourceset

debc242

more refactoring to separate vulkan proc tables

c719b8a

more wiring of things

c5ed6a7

everythign except vulkan functions pass

9b799f7

fix gn issues

047d4ea

all that is left is cleaning up

07d8b8f

iskakaushik force-pushed the engine_vma branch from 6c08717 to 07d8b8f Compare October 27, 2022 15:18

iskakaushik commented Oct 27, 2022

View reviewed changes

ci/licenses_golden/licenses_flutter Show resolved Hide resolved

iskakaushik added 5 commits October 27, 2022 11:38

fix obvious failures

2d2cf68

fix fuchsia things

aca1ac5

fix licenses

c0f955c

everything except lint

f224125

ignore context_vk lint

b4ffd7c

fix fuchsia stuff

75b842b

iskakaushik requested a review from chinmaygarde October 27, 2022 18:04

iskakaushik changed the title ~~[WIP] Wire Flutter's own VulkanMemoryAllocator implementation~~ [Impeller] Wire Flutter's own VulkanMemoryAllocator implementation Oct 27, 2022

flutter-dashboard bot added the platform-fuchsia label Oct 27, 2022

iskakaushik self-assigned this Oct 27, 2022

iskakaushik requested review from dnfield and bdero October 27, 2022 18:16

chinmaygarde approved these changes Oct 27, 2022

View reviewed changes

iskakaushik added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 27, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 27, 2022

fix clang-tidy problems

300e561

iskakaushik added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 27, 2022

auto-submit bot merged commit ff05a31 into flutter:main Oct 27, 2022

engine-flutter-autoroll mentioned this pull request Oct 27, 2022

Roll Flutter Engine from d90e5b7372a3 to b5c52563bbb6 (7 revisions) flutter/flutter#114198

Merged

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

ff05a31f5 [Impeller] Wire Flutter's own VulkanMemoryAllocator imple…

8281098

…mentation (flutter/engine#37018)

engine-flutter-autoroll mentioned this pull request Oct 28, 2022

Roll Flutter Engine from d90e5b7372a3 to 0e52894f30f7 (21 revisions) flutter/flutter#114248

Closed

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

ff05a31f5 [Impeller] Wire Flutter's own VulkanMemoryAllocator imple…

eaf94aa

…mentation (flutter/engine#37018)

engine-flutter-autoroll mentioned this pull request Oct 28, 2022

Roll Flutter Engine from d90e5b7372a3 to 774cb34d3ff5 (27 revisions) flutter/flutter#114258

Merged

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

ff05a31f5 [Impeller] Wire Flutter's own VulkanMemoryAllocator imple…

c497686

…mentation (flutter/engine#37018)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] Wire Flutter's own `VulkanMemoryAllocator` implementation #37018

[Impeller] Wire Flutter's own `VulkanMemoryAllocator` implementation #37018

Uh oh!

iskakaushik commented Oct 25, 2022 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Oct 25, 2022

Uh oh!

Uh oh!

chinmaygarde left a comment

Uh oh!

chinmaygarde Oct 27, 2022

Uh oh!

chinmaygarde Oct 27, 2022

Uh oh!

chinmaygarde Oct 27, 2022

Uh oh!

chinmaygarde Oct 27, 2022

Uh oh!

iskakaushik Oct 27, 2022

Uh oh!

chinmaygarde Oct 27, 2022

Uh oh!

iskakaushik Oct 27, 2022

Uh oh!

chinmaygarde Oct 27, 2022

Uh oh!

chinmaygarde Oct 27, 2022

Uh oh!

iskakaushik Oct 27, 2022

Uh oh!

auto-submit bot commented Oct 27, 2022

Uh oh!

zanderso commented Oct 27, 2022

Uh oh!

Uh oh!


		namespace flutter {

		class FlutterSkiaVulkanMemoryAllocator : public skgpu::VulkanMemoryAllocator {

[Impeller] Wire Flutter's own VulkanMemoryAllocator implementation #37018

[Impeller] Wire Flutter's own VulkanMemoryAllocator implementation #37018

Uh oh!

Conversation

iskakaushik commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Oct 25, 2022

Uh oh!

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!

auto-submit bot commented Oct 27, 2022

Uh oh!

zanderso commented Oct 27, 2022

Uh oh!

Uh oh!

[Impeller] Wire Flutter's own `VulkanMemoryAllocator` implementation #37018

[Impeller] Wire Flutter's own `VulkanMemoryAllocator` implementation #37018

iskakaushik commented Oct 25, 2022 •

edited

Loading