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

Add keepalive cache to keep objects stored as void* from being gc'ed. #250

Merged

Conversation

mCodingLLC
Copy link

Fixes: #248

Here is an alternative fix for #248. This approach keeps a cache of objects stored as void pointers so that the corresponding Python objects are not garbage collected before they are needed. The cache is cleared every new_frame(). The cache is not threadsafe, but I assume pyimgui does not intend to be threadsafe. The cache could cause memory use to increase without bound if the user does something nonsensical, e.g.

atlas = ... #some _FontAtlas object
while True:
    atlas.texture_id = 0 # copies of 0 will keep getting added to the cache until new_frame() is called.

I chose the cache to be a list because it would be the fastest and easiest option.
Another option would be to use a dict of id(object) -> object, which would be slower but not suffer from the memory issue above.

imgui/core.pyx Outdated Show resolved Hide resolved
@KinoxKlark
Copy link
Member

Indeed, pyimgui does not intent to be thread-safe. DearImgui itself is not thread-safe. It is the responsibility of the user to take care of that if it needs it. So I don't think this is an issue here.

Thank you very much for your help!

I added test cases to reproduce issue #248. I also changed _keepalive_cache to be inside _ImGuiContext, this leads to some refactoring of context management. Unfortunately, it seems that I can't push my changes to your branch. Can you grant me access please ? (https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork)

@mCodingLLC
Copy link
Author

Hmm, I have (and had) "Allow edits by maintainers" checked, so you should have access.

@learn-more
Copy link
Collaborator

Indeed, pyimgui does not intent to be thread-safe. DearImgui itself is not thread-safe. It is the responsibility of the user to take care of that if it needs it. So I don't think this is an issue here.

Thank you very much for your help!

I added test cases to reproduce issue #248. I also changed _keepalive_cache to be inside _ImGuiContext, this leads to some refactoring of context management. Unfortunately, it seems that I can't push my changes to your branch. Can you grant me access please ? (https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork)

afaik editing a contributors branch only work with https, not over ssh.

@KinoxKlark
Copy link
Member

Hmm, I have (and had) "Allow edits by maintainers" checked, so you should have access.

Yes indeed, my bad. Apparently, this was a bug with the push command.

Here is my proposal for inserting _keepalive_cache inside _ImGuiContext. I had to refactor a bit _ImGuiContext to be able to retrieve an instance of its python version. Originally we created a new instance from cpp pointer every time we wanted to work with it. I hope this is not too much of an overload.

What do you think about it?

@KinoxKlark
Copy link
Member

Thanks for your help!

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.

None yet

4 participants