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

Use Arc<ScipPtr> in place of all scip pointer uses #155

Merged
merged 10 commits into from
Oct 21, 2024
Merged

Conversation

mmghannam
Copy link
Member

No description provided.

src/scip.rs Show resolved Hide resolved
src/scip.rs Show resolved Hide resolved
src/solution.rs Show resolved Hide resolved
src/scip.rs Show resolved Hide resolved
This was referenced Oct 18, 2024
@mmghannam
Copy link
Member Author

mmghannam commented Oct 18, 2024

There's a memory leak that I traced back to the tests that use model.clone_for_plugins() I can't seem to figure it out. I think the way the callbacks are implemented I'm not decrementing dropping the Arc correctly. @Andful any idea what could be wrong here?

@Andful
Copy link

Andful commented Oct 21, 2024

Yeah, looking at it seemed like there is a memory leak. From what I saw, you double Box the trait. I think once is enough. But I have to look into it deeper. I would make an issue for now, and in the future, make a different pull request for it.

@mmghannam
Copy link
Member Author

mmghannam commented Oct 21, 2024

Yeah, looking at it seemed like there is a memory leak. From what I saw, you double Box the trait. I think once is enough. But I have to look into it deeper. I would make an issue for now, and in the future, make a different pull request for it.

I think I did that because I had compilation issues without this indirection. But maybe I did something wrong back then. Anyway, I'm hesistant to merge this with this memory leak but it's limited to the use of callbacks which is not the most common use case and it will enable much safer use of the library. So let's merge it and deal with the memory leaks later as you suggested.

Update: I added issue #159 to track this memory leak.

@mmghannam mmghannam marked this pull request as ready for review October 21, 2024 14:25
@mmghannam mmghannam merged commit a311dd2 into main Oct 21, 2024
4 of 5 checks passed
@mmghannam mmghannam deleted the arc-scipptr branch October 21, 2024 14:25
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