Upgrade Vulkan Memory Allocator, use Volk on Android #51524
Conversation
|
Needs to be rebased on master for fixing CI (after #51523) |
RandomShaper
force-pushed
the
faster_vma
branch
2 times, most recently
from
3d7bf5e
to
9753e5e
Compare
|
Android fails: We use the Vulkan headers from the NDK, which might be too old :/ And getting newer headers would require updating the NDK and thus fixing #44055. #51516 may help with the Android issue though, we'll see. |
RandomShaper
force-pushed
the
faster_vma
branch
2 times, most recently
from
a65574c
to
c1ce1fd
Compare
It's not just NDK Vulkan headers, Android is using VMA implementation for NDA as well (In #51516, it's doing it only if volk is disabled). |
|
So based on #51516, actually Android uses its own version of VMA in See #37745 for context, CC @pouleyKetchoupp. |
RandomShaper
force-pushed
the
faster_vma
branch
from
c1ce1fd
to
0dce931
Compare
|
@akien-mga, I've addressed your feedback. Also, note that I've changed the commit message. I didn't quite like VMA, which sounded to me more like Virtual Memory Address and the Vulkan word may be useful for potential grepping. |
|
So Android is still not happy, probably because we're still using the old NDK Vulkan headers by default. I don't remember why we do that, would need to check Git history. But maybe we can get away with not doing that and using our vendored copy of the headers? i.e. something like this: diff --git a/drivers/vulkan/SCsub b/drivers/vulkan/SCsub
index 3e0f5788c3..5db3f4a5b4 100644
--- a/drivers/vulkan/SCsub
+++ b/drivers/vulkan/SCsub
@@ -10,19 +10,8 @@ if env["use_volk"]:
env.AppendUnique(CPPDEFINES=["USE_VOLK"])
env.Prepend(CPPPATH=[thirdparty_volk_dir])
-if env["platform"] == "android" and not env["use_volk"]:
- # Use NDK Vulkan headers
- ndk_vulkan_dir = env["ANDROID_NDK_ROOT"] + "/sources/third_party/vulkan/src"
- thirdparty_includes = [
- ndk_vulkan_dir,
- ndk_vulkan_dir + "/include",
- ndk_vulkan_dir + "/layers",
- ndk_vulkan_dir + "/layers/generated",
- ]
- env.Prepend(CPPPATH=thirdparty_includes)
-else:
- # Use bundled Vulkan headers
- env.Prepend(CPPPATH=[thirdparty_dir, thirdparty_dir + "/include"])
+# Use bundled Vulkan headers
+env.Prepend(CPPPATH=[thirdparty_dir, thirdparty_dir + "/include"])
if env["platform"] == "android":
env.AppendUnique(CPPDEFINES=["VK_USE_PLATFORM_ANDROID_KHR"])
|
It seems to building fine with system loader and our headers. Not sure why it was done it this way (Android Vulkan support was added like this), Android use custom loader, but headers should be standard. |
|
Apparently we were using the NDK files because upstream Vulkan Loader doesn't support Android formally: KhronosGroup/Vulkan-Loader#96. But now that we switched to volk, this should no longer be needed, so we can remove that code. |
RandomShaper
force-pushed
the
faster_vma
branch
2 times, most recently
from
0aeb212
to
68aa484
Compare
|
|
|
Is it OK that without volk the VMA is restricted to Vulkan 1.0? Maybe that's not needed it targeting a more recent API level or using a newer NDK. Not familiar with the status of these things at this point. |
Why is this needed? Also as it's implemented now it will affect all platforms, e.g. iOS (which defaults to FWIW, we currently target Vulkan 1.1. The current PR builds fine for me for Android with |
|
I've added that because otherwise I was getting linking errors on the CI about missing symbols, corresponding to Vulkan functions ending with 2. I checked and they belong to Vulkan 1.1. I don't know what to do... |
We no longer build the Vulkan loader, and volk lets us load it dynamically. Roblox uses volk on Android so it should work well for us too.
|
We can use volk by default on Android, I'll make a separate PR to fix this up and you can rebase afterwards. |
|
Well slow CI will make this an overnight project, but once #51592 is merged, you can rebase this PR and add: diff --git a/COPYRIGHT.txt b/COPYRIGHT.txt
index 5bd67960da..ef444721b2 100644
--- a/COPYRIGHT.txt
+++ b/COPYRIGHT.txt
@@ -453,7 +453,7 @@ License: Apache-2.0
Files: ./thirdparty/vulkan/vk_mem_alloc.h
Comment: Vulkan Memory Allocator
-Copyright: 2017-2019, Advanced Micro Devices, Inc.
+Copyright: 2017-2021, Advanced Micro Devices, Inc.
License: Expat
Files: ./thirdparty/wslay/
diff --git a/drivers/vulkan/SCsub b/drivers/vulkan/SCsub
index ab45863f5b..8fe75367a8 100644
--- a/drivers/vulkan/SCsub
+++ b/drivers/vulkan/SCsub
@@ -36,6 +36,10 @@ if env["use_volk"]:
thirdparty_sources_volk = [thirdparty_volk_dir + "/volk.c"]
env_thirdparty_volk.add_source_files(thirdparty_obj, thirdparty_sources_volk)
+elif env["platform"] == "android":
+ # Our current NDK version only provides old Vulkan headers,
+ # so we have to limit VMA.
+ env_thirdparty_vma.AppendUnique(CPPDEFINES=["VMA_VULKAN_VERSION=1000000"])
env_thirdparty_vma.add_source_files(thirdparty_obj, thirdparty_sources_vma)
And this should be good to merge. |
RandomShaper
force-pushed
the
faster_vma
branch
from
68aa484
to
7b7e17a
Compare
|
I've rebased on top of your PR, which should save a little time if everything goes well. |
akien-mga
changed the title
|
Now this is finally relevant: #51524 (comment) |
|
Thanks! |
This is about #48978.
My profiling has led me to the Vulkan Memory Allocator (VMA) class as the culprit. Then I've found this: GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator#178
What this PR does is take the new experimental, faster implementation from https://github.com/GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator/tree/feature-small-buffers and do a couple minor changes needed to adapt to the new implementation.
I've done my measurements by using slightly modified versions of the MRP provided in the original issue, adapted to work in the current Godot 4 and to be editor scripts: ClassProfiling.zip
Now, the numbers (tested on Windows 10 x64 with locally built release binaries of the Godot editor, doing 1000 iterations):
Let's see it in a more convenient way, comparing before and after between a set of heavy cases:
As you can see, some are slightly worsened, but I'd say that can be considered noise.
If the work-in-progress new VMA is considered stable enough for the current development stage of Godot 4, this PR could be merged, knowing that we'll need to update the VMA when 3.0.0 is stable; or we can just wait until that happens, keeping the current worse performance in the meantime.