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

Memory management in Variants #502

Merged
merged 7 commits into from
Feb 26, 2025
Merged

Conversation

JulienDoerner
Copy link
Member

fixes #493:

The abstractConditions added a property to the candidate on the rejection. Up to now it only adds the key Rejected. I added the module name as the default Value for the flag.

Also the variant (in which this key is stored), does not have the right destructor. If it is deleted, the type of the variant was not passed to the clear function. Therefore no deletion of the variant was possible. This leaded to the bug in the memory consumption.

Copy link
Member

@lukasmerten lukasmerten left a comment

Choose a reason for hiding this comment

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

I have not yet checked if it fixes the actual bug, but in principle it looks good to me. When I have verified the fix and my comments are addressed, I am happy to merge.

@JulienDoerner
Copy link
Member Author

The error also occurred for setting a property to a candidate, but only if the property is of type STRING.

for i in range(10_000_000): 
  c = Candidate() 
  c.setProperty("k", "str")

will give the same issue as discussed before. With the explicit delete this issue is solved.

The only remaining point is related to the AssocVector.

Python gives the warning
swig/python detected a memory leak of type 'Loki::AssocVector< std::string,crpropa::Variant > *', no destructor found.
when running the following example:

 c = Candidate()
 c.properties 
 c.isActive() 

One solution could be to change the AssocVector to std::map which should have the same functionality, or does someone no a difference why we should keep the AssocVector?

@rafaelab
Copy link
Member

Hi @JulienDoerner
In the last exchange you said there is a problem with AssocVector.
Is that fixed? Do you think the PR is good to go with this issue unaddressed?

@JulienDoerner
Copy link
Member Author

Hey @rafaelab,

the AssocVector gives a warning swig/python detected a memory leak of type 'Loki::AssocVector< std::string,crpropa::Variant > *', no destructor found.
At the moment I have not found any real problem, therefore I would suggest leaving it as it is for the moment.

The underlying problem from #493 is solved with the fixes here.

@rafaelab
Copy link
Member

Fine with me. But apparently there are a few comments by @lukasmerten that were not addressed (or at least, they were not marked as resolved, such that I cannot approve the PR).

@JulienDoerner
Copy link
Member Author

Hey @rafaelab,
There was only one question about the usage left. I marked it as resolved, and you should be able to approve it now.

@JulienDoerner JulienDoerner dismissed lukasmerten’s stale review February 26, 2025 15:02

Changes adressed. Aproved by @rafaelab.

@JulienDoerner JulienDoerner merged commit c5ec112 into CRPropa:master Feb 26, 2025
3 of 4 checks passed
@JulienDoerner JulienDoerner deleted the issue_493 branch March 3, 2025 07:24
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.

Memory management in BreakingCondition
3 participants