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

MPI resource handling #30

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

MPI resource handling #30

wants to merge 11 commits into from

Conversation

lkotipal
Copy link
Contributor

@lkotipal lkotipal commented Feb 13, 2024

  • finalize checks if communicators and datatypes are null before freeing, and sets them accordingly to prevent double frees.
  • Destructor calls finalize. finalize can still be called manually in case object goes out of scope after MPI_finalize
  • Implement copy constructor correctly duplicating MPI communicator and datatypes.
  • Implement swap, and via that move constructor and assignment.
  • Delete copy assignment since it's horrible memory-wise (particularly with copy-swap) and its use is likely a mistake anyway.

@lkotipal lkotipal marked this pull request as ready for review February 13, 2024 16:47
Copy link
Contributor

@ursg ursg left a comment

Choose a reason for hiding this comment

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

Looking good. Definitely nice to get rid of those warnings (and to feel a bit better about not potentially wasting performance there).

fsgrid.hpp Show resolved Hide resolved
Copy link
Contributor

@ursg ursg left a comment

Choose a reason for hiding this comment

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

I know I approved this already, but double-approval looks even better!

Copy link
Contributor

@markusbattarbee markusbattarbee left a comment

Choose a reason for hiding this comment

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

🤔

fsgrid.hpp Show resolved Hide resolved
fsgrid.hpp Show resolved Hide resolved
fsgrid.hpp Show resolved Hide resolved
@iljah
Copy link

iljah commented Sep 26, 2024

since it's horribly slow

How slow is it?..

@lkotipal
Copy link
Contributor Author

lkotipal commented Oct 1, 2024

@iljah good point actually. I have absolutely not tested it, and it's more of an issue of memory footprint anyway for the canonical method. That would be constructing a temporary copy of the grid copied from and swapping with the grid copied to, meaning we're doing a whole bunch of allocation and holding an extra grid in memory.

@kstppd
Copy link
Contributor

kstppd commented Oct 1, 2024

Although my opinion was not asked I agree with Leo. Although letting the option be there from the viewpoint of correctness makes sense this library is used only by Vlasiator and Vlasiator has special needs so this acts like a deterrent to the future coder.

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.

5 participants