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

156: footprint types that are not serializable #157

Merged
merged 14 commits into from
Nov 24, 2020

Conversation

cz4rs
Copy link
Contributor

@cz4rs cz4rs commented Nov 19, 2020

fixes #156

current pain points in vt:

  • Open MPI types

  • std::list iterators

note:

  • general solution for any non-serializable type in a container would be best

@PhilMiller
Copy link
Member

I'd really like to see that countBytes logic applied to any non-serializable type in a container. Did you see my suggestion for a potentially appropriate type trait? It would go on the enable_if for the footprinting-specific container serialize functions

@cz4rs
Copy link
Contributor Author

cz4rs commented Nov 19, 2020

I'd really like to see that countBytes logic applied to any non-serializable type in a container. Did you see my suggestion for a potentially appropriate type trait? It would go on the enable_if for the footprinting-specific container serialize functions

Yes, I have seen it, but I was unable to get it to work so far (various errors at the dispatch level). Let me push a cleaned up example for easier discussion.

@cz4rs
Copy link
Contributor Author

cz4rs commented Nov 19, 2020

ed726eb and b41585c are two main lines that I've tried, but they are both not working - resulting in the same error as with the serializer completely missing

@cz4rs cz4rs force-pushed the 156-footprint-non-serializable-elements branch 3 times, most recently from 5cd2344 to 20261d4 Compare November 20, 2020 23:56
@cz4rs cz4rs force-pushed the 156-footprint-non-serializable-elements branch from 20261d4 to e91b3dd Compare November 21, 2020 00:45
tests/unit/test_footprinter.cc Outdated Show resolved Hide resolved
@cz4rs cz4rs force-pushed the 156-footprint-non-serializable-elements branch from 478e554 to 9b85a23 Compare November 23, 2020 12:22
@cz4rs cz4rs force-pushed the 156-footprint-non-serializable-elements branch from 9b85a23 to cc0f0d4 Compare November 23, 2020 13:05
@PhilMiller PhilMiller force-pushed the 156-footprint-non-serializable-elements branch from de42001 to f650ecc Compare November 24, 2020 00:17
@PhilMiller
Copy link
Member

Ok, I've gotten the type trait stuff working for the serializeArray case. However, I realize that more sensible thing to do is maybe solve this at a slightly lower level, in the Traverse machinery or below, so that it works for containers other than std::vector more automatically.

@PhilMiller PhilMiller force-pushed the 156-footprint-non-serializable-elements branch from da084ef to 739b391 Compare November 24, 2020 01:21
@PhilMiller PhilMiller changed the title 156: footprint containers whose elements are not serializable 156: footprint type that are not serializable Nov 24, 2020
static bool firstCall = true;
if (firstCall) {
firstCall = false;
// optionally print a warning here
Copy link
Member

Choose a reason for hiding this comment

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

We need to decide what to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hard to say how much additional value a separate warning would provide, maybe expanding the first (unconditional) print on the entrance would be sufficient (e.g. adding typeid)?

Copy link
Member

Choose a reason for hiding this comment

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

My thinking is that we may want a printed warning here in much broader circumstances than when we have full debug printing turned on. Perhaps aligned with CMAKE_BUILD_TYPE=Debug or such.

Copy link
Member

Choose a reason for hiding this comment

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

Regardless, both this debug print and the ones below should probably have a name from typeid incorporated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that printing typeid would be good here.

@PhilMiller
Copy link
Member

I just tested that this passes all the footprinting tests in vt.

@PhilMiller PhilMiller changed the title 156: footprint type that are not serializable 156: footprint types that are not serializable Nov 24, 2020
@cz4rs
Copy link
Contributor Author

cz4rs commented Nov 24, 2020

@PhilMiller this looks good to me, I can see that the PR check is complaining about signatures missing (in your commits) - feel free to force push the amendments

@lifflander this probably requires you review as a third party

edit: after giving it a second thought, this probably needs an explicit mention in the documentation, because now user will always be able to footprint stuff, but might be unaware about a silent fallback to this approach

@cz4rs cz4rs marked this pull request as ready for review November 24, 2020 17:20
@cz4rs cz4rs force-pushed the 156-footprint-non-serializable-elements branch from 38182ae to 151b1c8 Compare November 24, 2020 17:35
Copy link
Contributor

@lifflander lifflander left a comment

Choose a reason for hiding this comment

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

Overall this looks good. A few minor points about formatting fixes

static bool firstCall = true;
if (firstCall) {
firstCall = false;
// optionally print a warning here
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that printing typeid would be good here.

src/checkpoint/dispatch/dispatch_serializer_nonbyte.h Outdated Show resolved Hide resolved
src/checkpoint/container/raw_ptr_serialize.h Outdated Show resolved Hide resolved
@cz4rs cz4rs force-pushed the 156-footprint-non-serializable-elements branch from 0b4e817 to 0d07cd6 Compare November 24, 2020 20:47
@cz4rs cz4rs merged commit fb14b1c into develop Nov 24, 2020
cz4rs referenced this pull request in DARMA-tasking/vt Feb 10, 2021
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.

footprint types that are not serializable
3 participants