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

Fix a command buffer leak that occurs in D3D12 #100151

Merged

Conversation

alessand10
Copy link
Contributor

@alessand10 alessand10 commented Dec 7, 2024

This PR should fix: #94251

Memory allocated for command buffers in DX12 is never freed. In Vulkan, freeing the command pool also frees the command buffers that have been allocated within the pool. This fix mirrors Vulkan behavior by having each command pool track their respective command buffers. When a command pool is freed, its command buffers are as well.

@alessand10 alessand10 requested a review from a team as a code owner December 7, 2024 18:29
@AThousandShips AThousandShips added bug platform:windows topic:rendering cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Dec 7, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Dec 7, 2024
Copy link
Member

@clayjohn clayjohn 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 to me.

I would like @DarioSamo or @RandomShaper to take a quick look as well before we merge

@RandomShaper
Copy link
Member

The rationale makes sense. However, the implementation fails to follow the 'as close to zero allocations as possible' mindset. I think you could use SelfList to achieve that.

@clayjohn
Copy link
Member

@alessand10 Could you make the change requested by RandomShaper? I think after doing that, this should be ready to merge

@alessand10 alessand10 force-pushed the fix-dx12-command-buffer-leak branch from 6ed73b1 to 080cd48 Compare December 17, 2024 14:40
@alessand10 alessand10 requested review from a team as code owners December 17, 2024 14:40
@clayjohn clayjohn removed request for a team December 17, 2024 15:43
@alessand10 alessand10 closed this Dec 17, 2024
@alessand10 alessand10 force-pushed the fix-dx12-command-buffer-leak branch from 10021a3 to 2b7ea62 Compare December 17, 2024 15:53
@AThousandShips
Copy link
Member

You accidentally wiped your branch when trying to restore, to get back to before the rebase please use git reset --hard 6ed73b1 and try again

@alessand10
Copy link
Contributor Author

@AThousandShips Sorry for the trouble. Will I need to create a new PR?

@AThousandShips
Copy link
Member

No you can solve this one, try:

git reset --hard 6ed73b1

And then try to rebase again, and then finally:

git push --force

I'd help but I'm away from my computer right now

@alessand10 alessand10 reopened this Dec 17, 2024
@clayjohn clayjohn self-requested a review December 17, 2024 17:12
@clayjohn
Copy link
Member

Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

@alessand10 alessand10 force-pushed the fix-dx12-command-buffer-leak branch from 3d54eb5 to 0095d4d Compare December 17, 2024 17:28
@alessand10
Copy link
Contributor Author

alessand10 commented Dec 17, 2024

@clayjohn @AThousandShips Thank you for your help and guidance!

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Sweet!

Just left a little suggestion, but feel free to ignore it. I consider this is fine the way it already is.

@adamscott adamscott changed the title Fix a command buffer leak that occurs in dx12 Fix a command buffer leak that occurs in D3D12 Dec 17, 2024
@alessand10 alessand10 force-pushed the fix-dx12-command-buffer-leak branch 4 times, most recently from 83cc8e7 to 83cc152 Compare December 17, 2024 19:53
@alessand10 alessand10 force-pushed the fix-dx12-command-buffer-leak branch from 83cc152 to 9ea97c0 Compare December 17, 2024 19:55
Copy link
Member

@clayjohn clayjohn 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 your quick responses to feedback! That was a bit of a whirlwind day. But it looks great to merge now

@alessand10
Copy link
Contributor Author

@clayjohn My second PR was definitely more chaotic than I was aiming for, but I appreciate everyone's help and I'm looking forward to learning more about the engine and contribution process. Thanks again!

@akien-mga akien-mga merged commit 7a70efc into godotengine:master Dec 17, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release platform:windows topic:rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Pages in use exist at exit in PagedAllocator" on exit using D3D12
6 participants