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

Implementing weakref and cloning #539

Merged
merged 93 commits into from
Oct 10, 2024
Merged

Implementing weakref and cloning #539

merged 93 commits into from
Oct 10, 2024

Conversation

MicahGale
Copy link
Collaborator

@MicahGale MicahGale commented Sep 10, 2024

Description

This focused on making the copying experience better.

First this implemented weakref.ref for self._problem. This breaks a cyclic/recursive memory reference. This was bad for two reasons. First It made problems nearly impossible to garbage collect due to the cyclic references. Secondly it made copy.deepcopy very slow and memory costly due to the near infinite recursion in copying. To use a weakref.ref you have to call it each. So a new attribute _problem_ref was added to contain this. _problem was made a property to hide the dereferencing operation. weakref.ref can't be pickled.

This pickling conundrum was handled through __getstate__ and __setstate__. When pickling the weakref would be removed. When unpickling though all these refs were gone. There was no easy way to ensure the refs could be built at unpickling because of the order that objects are unpickled. Rather a hook was added to all contains cells, surfaces, etc. which would check if it had been unpickled and if so quickly recreate all the weakrefs.

The other issue then was that a deepcopy of a weakref still points to the same object. So the deepcopy` method was overwritten for a problem so that the references would be to the new problem, and not the old one. For all other objects when they are copied on their own they will point to the same problem as before.

All of this work sounded like clone. So that was implemented at the same time. This was done in an OpenMC like way.

Fixes #514, fixes #469.

Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@MicahGale MicahGale self-assigned this Sep 26, 2024
@MicahGale MicahGale added bugs A deviation from expected behavior that does not reach the level of being reportable as an "Error". feature request An issue that improves the user interface. labels Sep 26, 2024
@MicahGale MicahGale marked this pull request as ready for review October 8, 2024 21:32
@MicahGale MicahGale marked this pull request as draft October 8, 2024 21:46
@MicahGale
Copy link
Collaborator Author

This needs more testing.

@MicahGale MicahGale marked this pull request as ready for review October 9, 2024 13:29
@MicahGale
Copy link
Collaborator Author

Hopefully final round of reviews.

I:

  1. Implemented collection level default numbers
  2. documented how this works
  3. created more testing!

@MicahGale MicahGale added the code improvement A feature request that will improve the software and its maintainability, but be invisible to users. label Oct 9, 2024
This was referenced Oct 9, 2024
@MicahGale MicahGale merged commit 4f6144d into develop Oct 10, 2024
20 checks passed
@MicahGale MicahGale deleted the weakref branch October 10, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugs A deviation from expected behavior that does not reach the level of being reportable as an "Error". code improvement A feature request that will improve the software and its maintainability, but be invisible to users. feature request An issue that improves the user interface. performance 🐌 Issues related to speed and memory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leaks with copy.deepcopy Add Simple method to make deepcopies
2 participants