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

Cleanup at end of transaction to prevent memory leaks #15808

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

krisselden
Copy link
Contributor

Cleanup at end of transaction to prevent memory leaks when objects that are rendered are retained.

Fix #15803

@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2017

@GavinJoyce - Can you review also?

Copy link
Member

@stefanpenner stefanpenner left a comment

Choose a reason for hiding this comment

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

LGTM: Reviewed, and spoke with @krisselden about this. :shipit:

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

This generally looks good to me, thanks for tackling it! I do find this coding style slightly confusing though (totally a pre-existing issue, and not something new introduced in this PR).

It would go a very long way if we had a comment that explained the general idea of what is going on. Basically documenting the setup, addKey, checkKey, and cleanup functions and how they play into the general transaction system here...

leaks when objects that are rendered are retained.

Fix #15803
@krisselden krisselden force-pushed the fix-double-assert-render-leak branch from 811d3ee to 316c7c6 Compare November 3, 2017 03:23
@krisselden
Copy link
Contributor Author

@rwjblue I think I made the code clearer, it doesn't diff as well, but can you take a look to see if you understand it better now?

@GavinJoyce
Copy link
Member

GavinJoyce commented Nov 3, 2017

This looks good to me (and I agree that the code is now easier to understand)

@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2017

Thank you @krisselden! This is very much easier for me to understand (and likely better factored too boot).

@rwjblue rwjblue merged commit e519985 into master Nov 3, 2017
@rwjblue rwjblue deleted the fix-double-assert-render-leak branch November 3, 2017 11:07
@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2017

I believe that this needs to make its way into a 2.16 release once we have vetted it on 2.17 betas...

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

Successfully merging this pull request may close these issues.

5 participants