Optimize Utilities::pack/unpack for empty objects. #13854
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I implemented this idea in the last commit. |
|
Part of #13208. |
kronbichler
approved these changes
May 30, 2022
There was a problem hiding this comment.
This looks good, especially the documentation is really useful. In my opinion it feels safe for the release. The only problem I can anticipate is whether some compiler might get into trouble with some sizeof, but that tuple type is so basic that I can't imagine it being the case.
include/deal.II/base/utilities.h
Outdated
| * `T` satisfies `std::is_trivially_copyable`. | ||
| * - Finally, if the type `T` of the object to be packed is std::tuple<> | ||
| * (i.e., a tuple without any elements as indicated by the empty argument | ||
| * list) and if not compression is requested, then this |
There was a problem hiding this comment.
Suggested change
| * list) and if not compression is requested, then this | |
| * list) and if no compression is requested, then this |
include/deal.II/base/utilities.h
Outdated
Comment on lines
597
to
598
| * this by providing a type which, after packing, results in an empty | ||
| * string. |
There was a problem hiding this comment.
I do not really understand that last part of the clause from a language point of view. Can you have a look?
kronbichler
reviewed
May 30, 2022
| // we want it to be as small an object as possible. Using | ||
| // std::tuple<> is interpreted as an empty object that is packed | ||
| // down to a zero-length char array. | ||
| return EmptyType(); |
There was a problem hiding this comment.
clang-tidy complains about this one and the ones below,
/jenkins/workspace/dealii-tidy_PR-13854/include/deal.II/base/mpi_consensus_algorithms.h:1196:20: error: avoid repeating the return type from the declaration; use a braced initializer list instead [modernize-return-braced-init-list,-warnings-as-errors]
/jenkins/workspace/dealii-tidy_PR-13854/include/deal.II/base/mpi_consensus_algorithms.h:1248:20: error: avoid repeating the return type from the declaration; use a braced initializer list instead [modernize-return-braced-init-list,-warnings-as-errors]
/jenkins/workspace/dealii-tidy_PR-13854/include/deal.II/base/mpi_consensus_algorithms.h:1302:20: error: avoid repeating the return type from the declaration; use a braced initializer list instead [modernize-return-braced-init-list,-warnings-as-errors]
/jenkins/workspace/dealii-tidy_PR-13854/include/deal.II/base/mpi_consensus_algorithms.h:1356:20: error: avoid repeating the return type from the declaration; use a braced initializer list instead [modernize-return-braced-init-list,-warnings-as-errors]
|
I've addressed the points raised by @kronbichler, and I've also added a paragraph that describes what it means for a type to be "trivially copyable". |
kronbichler
approved these changes
May 30, 2022
mkghadban
pushed a commit
to OpenFCST/dealii
that referenced
this pull request
Sep 8, 2022
Optimize Utilities::pack/unpack for empty objects.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Not necessary for the release unless others feel like it would be particularly useful.]
This is another optimization to the pack()/unpack() functions: We can special case a truly empty object that will be packed down to zero bytes. The corresponding MPI sends/receives are then empty.
There is no C++ facility to determine whether a type is empty.
sizeof(T)always returns at least one, so one has to choose something for which we know that it really doesn't store anything even if its size is >0, and for that reason that type cannot be a user-defined type. I'm simply usingstd::tuple<>(i.e., a tuple with no elements) to designate that type. This patch does that and documents it.Why do I want to do that: In #13826 -- where I consider the consensus algorithms that typically have a send-request/reply-with-an-answer phases, but for the special case where no answer is required -- I need send a dummy object that is empty. In the patch, I just use a single-character
charobject, the smallest possible object in C++. After packing, that results in a one-byte message whose content we don't actually care about. It would be nice if we could shrink this to a zero-length message, but that requires that we have something thatpack()can shrink down to a zero-size buffer. This patch does that./rebuild