Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VulkanDeviceAlloc: Memorytype per slab #11744

Merged
merged 2 commits into from
Jan 23, 2019

Conversation

hrydgard
Copy link
Owner

See #11743 for the details. This fixes a wrong assumption we had about driver behavior.

@hrydgard hrydgard added this to the v1.8.0 milestone Jan 23, 2019
@hrydgard
Copy link
Owner Author

@unknownbrackets I'm gonna go ahead and merge this, since I think it's pretty safe. Let me know what you think when you have a sec.

@hrydgard hrydgard merged commit 8aea194 into master Jan 23, 2019
@hrydgard hrydgard deleted the vk-devicealloc-memorytype-per-slab branch January 23, 2019 20:31
Copy link
Collaborator

@unknownbrackets unknownbrackets left a comment

Choose a reason for hiding this comment

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

Makes sense, hopefully it doesn't make a big difference on the size increase, depends how often this happens...

-[Unknown]

@@ -76,6 +77,8 @@ bool VulkanTexture::CreateDirect(VkCommandBuffer cmd, int w, int h, int numMips,
if (allocator_) {
offset_ = allocator_->Allocate(mem_reqs, &mem_, Tag());
if (offset_ == VulkanDeviceAllocator::ALLOCATE_FAILED) {
vkDestroyImage(vulkan_->GetDevice(), image_, nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Destroy is called in the destructor which already does this. We should at least clear image_, but I'm personally not a big fan of repeating WET cleanup code in every error case, so I try to design things so they auto-cleanup and the code is easier to read.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

WET? Yeah I see your point, I'll clean it up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We Enjoy Typing. The opposite of DRY.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah :)

@@ -89,6 +92,7 @@ bool VulkanTexture::CreateDirect(VkCommandBuffer cmd, int w, int h, int numMips,

res = vkAllocateMemory(vulkan_->GetDevice(), &mem_alloc, NULL, &mem_);
if (res != VK_SUCCESS) {
ELOG("vkAllocateMemory failed: %s", VulkanResultToString(res));
_assert_msg_(G3D, res != VK_ERROR_TOO_MANY_OBJECTS, "Too many Vulkan memory objects!");
_assert_(res == VK_ERROR_OUT_OF_HOST_MEMORY || res == VK_ERROR_OUT_OF_DEVICE_MEMORY || res == VK_ERROR_TOO_MANY_OBJECTS);
vkDestroyImage(vulkan_->GetDevice(), image_, nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW, this one is also not necessary but at least clears image_ so is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants