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.

Sign up for GitHub

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

Jump to bottom

Fix leak where texel buffer is occasionally accidentally created during image-memory unbinding (#2341) #2345

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Fix leak where texel buffer is occasionally accidentally created during image-memory unbinding (#2341) #2345

merged 1 commit into from
Sep 25, 2024

Conversation

warmenhoven
Copy link
Contributor

@warmenhoven warmenhoven commented Sep 21, 2024 •
edited by billhollings
Loading

Fixes #2341.

cdavis5e
cdavis5e requested changes Sep 21, 2024
View reviewed changes
MoltenVK/MoltenVK/GPUObjects/MVKImage.mm Outdated Show resolved Hide resolved
MoltenVK/MoltenVK/GPUObjects/MVKImage.mm Outdated Show resolved Hide resolved
cdavis5e
cdavis5e requested changes Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@cdavis5e cdavis5e left a comment

Choose a reason for hiding this comment

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

Looks good. Just one small thing.

MoltenVK/MoltenVK/GPUObjects/MVKImage.mm Outdated Show resolved Hide resolved
@billhollings billhollings changed the title Release image from view from both destroy and destructor (#2341) Fix leak where texel buffer is occasionally accidentally created during image-memory unbinding (#2341) Sep 24, 2024
@billhollings billhollings mentioned this pull request Sep 24, 2024
Closed
billhollings
billhollings requested changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@billhollings billhollings left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR.

And thanks for all your incredible detective work! I appreciate you digging into this so thoroughly! Great catch!

LGTM. Just a couple of very minor style and robustness requests.

And when amending this, can you modify the commit message to match the updated fix, please? I was able to change the PR description, but I can't change the commit message itself.

Thanks.

MoltenVK/MoltenVK/GPUObjects/MVKImage.mm Outdated Show resolved Hide resolved
MoltenVK/MoltenVK/GPUObjects/MVKImage.mm Show resolved Hide resolved
@warmenhoven
Copy link
Contributor Author

I've updated the PR for the wording, the style, and the extra defensive check, and it's all running well for me. Thanks very much!

Out of curiosity, under what conditions was vkFreeMemory() being called so frequently on every frame?

I wish I could say I understood Granite well enough to explain what it's doing here. My exact test case is just running the Nintendo 64 RetroArch core using its Vulkan renderer.

billhollings
billhollings approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@billhollings billhollings left a comment

Choose a reason for hiding this comment

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

LGTM now. Thanks!

@billhollings billhollings merged commit b2d7ad6 into KhronosGroup:main Sep 25, 2024
6 checks passed
@warmenhoven warmenhoven deleted the warmenhoven/pr/2341 branch September 25, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdavis5e cdavis5e cdavis5e approved these changes

@billhollings billhollings billhollings approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GPU Memory leak on A12Z-based iPad Pro, ATV 4K 1st Gen
3 participants
@warmenhoven @billhollings @cdavis5e