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 a function to free allocated memory #52

Merged
merged 2 commits into from
May 12, 2023

Conversation

BadSingleton
Copy link
Contributor

Managed runtimes like C# P/Invoke requires this in order to be able to correctly free memory in managed code that was allocated by unmanaged code.

The method was only defined in copentime because it can be used on it's own but opentimlineio must be used with opentime.

@BadSingleton
Copy link
Contributor Author

This is a subset of #15. It doesn't solves the interning of strings, only exposes free so that managed runtimes can free memory from managed-land.

@meshula
Copy link
Member

meshula commented Apr 13, 2023

there's some complexity in COpentimelineIO about the memory management of retained objects and retained objects in containers. I'm slightly worried that if we expose something named "free" people might get excited and think that they can use to more easily manage retained COTIO objects, which isn't the case.

I'm thinking naming and documentation would help. Could you maybe in this thread, explain who is going to call the wrapped free and what they are going to be freeing?

@BadSingleton
Copy link
Contributor Author

Apologies for the delay. This is for managed runtimes like C# who can write all the bindings as managed code.
In an example case of C# where the bindings are coded in pure C# via P/Invoke, without an exposed free function, cstrings are a memory leak as managed strings are made by copying the cstring. Ownership by the managed runtime would also be a problem; as when garbage collected, they wouldn't be able to free the memory allocated by the native runtime.
You can also see this PR as a very basic subset of #15

@meshula
Copy link
Member

meshula commented May 11, 2023

I see, that makes sense. Two questions

(1) can you run git commit --amend --signoff on your repo and force push? That will clear up the DCO warning we can see in the CI. We can go ahead and land it at that point to unstuck you.

(2) My gut feeling is that maybe it's time to move ahead with the pattern in #15. Would that allow for anything interesting in your C# bindings, in terms of easier management of strings or whatever?

Managed runtimes like C# P/Invoke requires this in order to be able to
correctly free memory in managed code that was allocated by unmanaged
code.

The method was only defined in copentime because it can be used on it's
own but opentimlineio must be used with opentime.

Signed-off-by: Félix Bourbonnais <[email protected]>
@BadSingleton BadSingleton force-pushed the expose-free branch 2 times, most recently from 014f404 to 5787504 Compare May 12, 2023 13:20
@BadSingleton
Copy link
Contributor Author

BadSingleton commented May 12, 2023

Would that allow for anything interesting in your C# bindings, in terms of easier management of strings or whatever?

It would make this PR obsolete, which is a good thing

edit: if the otiostr_delete is exported/external visibility in #15, it would obsolete this PR

@meshula
Copy link
Member

meshula commented May 12, 2023

Cool, thanks!

@meshula meshula merged commit 832b92a into OpenTimelineIO:main May 12, 2023
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.

2 participants