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

Implement AlignedVector::shrink_to_fit() #15974

Merged
merged 3 commits into from Sep 20, 2023

Conversation

bergbauer
Copy link
Contributor

No description provided.

@marcfehling
Copy link
Member

/rebuild

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

Can you make sure this function is not called if someone had previously called replicate_across_communicator()?

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

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

Good once you have addressed @bangerth's comment and have added a change-log entry.

@bergbauer
Copy link
Contributor Author

Can you make sure this function is not called if someone had previously called replicate_across_communicator()?

Sure, see the last commit. @bangerth But wouldn't this logic apply to every non-const function of AlignedVector? As far as I understood the documentation of replicate_across_communicator() the vector should not be changed at all afterwards.

Comment on lines 688 to 691
/**
* Flag indicating if replicate_across_communicator() has been called.
*/
bool replicated_across_communicator;
Copy link
Member

Choose a reason for hiding this comment

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

Should we guard this with ifdef. Similar as we do in FEEval?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In FEEvaluationData we don't guard the declaration of the variables but all occurrences in definitions (also in asserts, initialization in constructors). Done.

Copy link
Member

Choose a reason for hiding this comment

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

Odd that the compiler does not warn that the variable is not initialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would leave it like this if the compiler does not complain, otherwise, the size of the vector is different for release and debug because of the additional member.

Comment on lines +478 to +480
DeclExceptionMsg(ExcAlignedVectorChangeAfterReplication,
"Changing the vector after a call to "
"replicate_across_communicator() is not allowed.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have introduced this exception message which we can use for all non-const functions in a follow-up.

Comment on lines +1471 to +1474
# ifdef DEBUG
Assert(replicated_across_communicator == false,
ExcAlignedVectorChangeAfterReplication());
# endif
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't need to guard this. Assert is only performed in debug mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In FEEvaluationData the Asserts are also guarded by #ifdef DEBUG. I can remove it if it is not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

OK. Let's keep it this way to be consistent.

@peterrum
Copy link
Member

@bangerth Any final comments? The requested assert has been added.

@bergbauer
Copy link
Contributor Author

Ping

I want to use this to release memory after mapping compression in NonMatching::MappingInfo.

@kronbichler kronbichler dismissed bangerth’s stale review September 20, 2023 08:21

Request has been addressed.

@kronbichler kronbichler merged commit 6b7e292 into dealii:master Sep 20, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants