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

emp::array uses dynamically allocated memory under the hood #408

Open
mmore500 opened this issue Jan 11, 2021 · 2 comments
Open

emp::array uses dynamically allocated memory under the hood #408

mmore500 opened this issue Jan 11, 2021 · 2 comments

Comments

@mmore500
Copy link
Member

Describe the bug
A clear and concise description of what the bug is.

emp::array uses std::vector as a backend. For single process jobs, this works fine. For multiprocessing jobs, this creates an issue when passing arrays in messages between processes. Messages will contain pointers to data that allocated on one process that another processes doesn't have.

To my knowledge, there isn't a fundamental reason emp::array is implemented using std::vector. Looking into a quick patch, I found that working with std::vector is more convenient because its iterator is a proper class and therefore you can inherit from it. std::array's iterator is /not/ a class -- it's just a pointer so you can't inherit from it. But with a little elbow grease we should be able to switch out emp::array's backend to be std::array instead. Perhaps @mercere99 can weigh in on this assessment?

mmore500 added a commit to mmore500/conduit that referenced this issue Jan 11, 2021
mmore500 added a commit to mmore500/conduit that referenced this issue Jan 11, 2021
@mercere99
Copy link
Member

The comments made clear that I used std::vector as the base intentionally (for debug mode only), but I have no recollection why. Have you tried swapping in std::array to see what goes wrong? From your comment above, it sounds like I was more easily able to derive from vector's iterator in order to identify when an iterator have become invalid. Conveniently, the lack of size changes makes this a much rarer situation in arrays.

I have no issues with you making a fix! And if you come up with a reason why it really needs to be vector, please update the docs to indicate. ;-)

@mmore500
Copy link
Member Author

I did notice those comments 🕵️, I'll take a shot at it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants