Skip to content

Commit

Permalink
- updated vk_mem_alloc.
Browse files Browse the repository at this point in the history
  • Loading branch information
@coelckers
coelckers committed Oct 17, 2021
1 parent 8149c3e commit eb9f752
Show file tree
Hide file tree
Showing 2 changed files with 15,434 additions and 4,940 deletions.
1 change: 1 addition & 0 deletions src/CMakeLists.txt
View file
Expand Up @@ -716,6 +716,7 @@ set( NOT_COMPILED_SOURCE_FILES
zcc-parse.c
zcc-parse.h
common/platform/win32/zutil.natvis
common/rendering/vulkan/thirdparty/vk_mem_alloc/vk_mem_alloc.natvis
)

set( VM_JIT_SOURCES
Expand Down

6 comments on commit eb9f752

@alexey-lysiuk
Copy link
Collaborator

@alexey-lysiuk alexey-lysiuk commented on eb9f752 Oct 17, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it fix out-of-memory handling?

The commit has the following issues:

  • It has a missing file, vk_mem_alloc.natvis, need to add this.
  • It needs newer Vulkan SDK, update to the latest one fixes it.
  • It triggers assertion failure inside allocator without the the following patch.
--- a/src/common/rendering/vulkan/system/vk_device.cpp
+++ b/src/common/rendering/vulkan/system/vk_device.cpp
@@ -220,6 +220,7 @@ void VulkanDevice::CreateAllocator()
 		allocinfo.flags = VMA_ALLOCATOR_CREATE_KHR_DEDICATED_ALLOCATION_BIT;
 	allocinfo.physicalDevice = PhysicalDevice.Device;
 	allocinfo.device = device;
+	allocinfo.instance = instance;
 	allocinfo.preferredLargeHeapBlockSize = 64 * 1024 * 1024;
 	if (vmaCreateAllocator(&allocinfo, &allocator) != VK_SUCCESS)
 		VulkanError("Unable to create allocator");
  • It requires bumping of macOS version requirement to 10.12 because of std::shared_mutex. Fixing all deprecation warnings is another task.

I can commit these changes if all mentioned things are acceptable.

@coelckers
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the missing file which I simply forgot to add before committing it worked out of the box for me with the existing Vulkan SDK I use. If there's anything needed I at least cannot detect it.

@Cacodemon345
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the macOS version requirement is to be upped to 10.12 it should also ensure the removal of those macOS-specific timer code in favour of clock_gettime.

@coelckers
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really sucks that Apple uses the standard C++ library to force OS upgrades onto their users.
They did the same thing with std::filesystem.

That said, Windows only supports std::shared_mutex from Vista and up - but that is because the OS primitive does not exist in XP so there's no way to implement it there.

@alexey-lysiuk
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conversation doesn't answer my questions though. Does allocator update solve out-of-memory handling? If so, do we accept it taking into account additional changes to make it work?

Regarding clock_gettime(), did you measure its cost? Old implementation using mach_absolute_time() was replaced solely because of that.

@coelckers
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I haven't tested it yet.
However, considering that the old version of this library was almost 1.5 years old there's likely other issues that got addressed. The new file is twice as large and does not make it easy to actually compare.

Please sign in to comment.