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

residue_for_atom should give a view instead instead of a copy #60

Open
egolep opened this issue Apr 13, 2022 · 2 comments
Open

residue_for_atom should give a view instead instead of a copy #60

egolep opened this issue Apr 13, 2022 · 2 comments

Comments

@egolep
Copy link

egolep commented Apr 13, 2022

I'm playing with both the bindings for Julia and Python, and while working with residues on a liposome, I found that, in the Julia version, Residue and residue_for_atom are making copies of the residue instead of giving a view, like in the Python version where a raed-only access is given. The problem is that the Julia version is unusable for me because of the memory consumption that comes with doing so much copies of potentially big data structures.
Is there a workaround that I haven't been able to find? Otherwise, I think that we should be using views or giving the possibility to choose if the function does a copy or not

@Luthaf
Copy link
Member

Luthaf commented Apr 14, 2022

There is indeed a difference between Python and Julia here. The core of the issue is that they use different garbage collection strategy, and Python ends up cleaning up much earlier, while Julia keeps stuff around until there is more memory pressure.

This made it so that handing out views in Julia could cause segfaults, if the view was pointing in memory that was re-allocated in the mean time:

atom = @view frame[3]

add_atom!(frame, Atom(...))
add_atom!(frame, Atom(...))
add_atom!(frame, Atom(...))
add_atom!(frame, Atom(...))

name(atom) # can segfault depending on the memory allocator behaviour

This is the reason why I went with a copy in Julia by default.

The proper way of solving this would be to emit an error in add_atom! if there are still references to atoms floating around. This could be done in C++, but I did not had the time to implement it for now.


At the same time, I understand the need to use views instead of copies in a lot of cases, and doing so without modifying the frame should be perfectly safe. #53 started to add back opt-in support for views, and I would be happy with a view=true parameter to Residue and residue_for_atom.

Is this something you could help implement? You can use #53 as an inspiration on how to do it.

@egolep
Copy link
Author

egolep commented Apr 14, 2022

I would be glad to try to implement that.
I'm in a pretty busy period so I'm afraid I'll have to work on this in my spare time, but I'll definitely try.
Thanks for the deeper explanation of the problem and for pointing me to #53

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

No branches or pull requests

2 participants