[WIP] Add infrastructure for state serialization

[sherm1]

This is the first crack at State serialization, which is not yet complete. This contains the necessary infrastructure to provide toXmlElement() and fromXmlElement() support for general types and (most important) for AbstractValue and Value<T>.

See TestXml.cpp for test cases forAbstractValue and Value<T>. State.toXmlElement() and State.fromXmlElement() exist and will work if all the contained AbstractValue types have those methods, or if they have stream extraction/insertion operators from which workable defaults an be created.

No State serialization tests are committed here yet. Those need to get written and debugged. @chrisdembia, if you want to work on this have a look and then we can discuss.

---

This change is 

[sherm1]

I don't like that toXmlElementHelper has to be used here. Would be friendlier for people writing these methods if it were just toXmlElement and the mysterious third parameter were hidden somewhere below.

[chrisdembia]

To clarify, you don't like that toXmlElementHelper has to be used anywhere (within toXmlElement() methods), right? Can you educate me as to why the third parameter is necessary for SFINAE?

[sherm1]

Yeah, the "Helper" method should just be needed in the internal implementation. Here it is appearing in user-written code for serializing.

The issue is that we normally want classes to have toXmlElement() methods. But when serializing an existing class (std::vector, say) you can't add a new method. So you have to write a free function toXmlElement(std::vector<T>) to serialize it. What if there is both a member named MyClass::toXmlElement() and a free function toXmlElement(MyClass)? I wanted to ensure that the member gets called. By template resolution rules, that true value is a better match for a bool than an int. So I arranged things so that the member function gets called from a Helper that takes a bool while the free function gets called from a Helper that takes an int. That one gets called if there's no bool competing with it because true gets promoted to int.

All that is still necessary; I'm just thinking it doesn't have to appear in user code. Use code could call some nicely-named method, then that method could invoke the Helper which would then pick out the right member or free function. I think that should be possible ...

[chrisdembia]

Thank you; that was very helpful. I also described this situation to @klshrinidhi, who may be interested in helping.

[sherm1]

Any other idea you guys have for the API would be welcome. It should be as straightforward as possible for someone to write serialization/deserialization methods for any class they want to stick into an AbstractValue in a State. I assume people will do it by copying from existing examples so it should be very predictable. The current API might be OK but it doesn't seem optimal.

[sherm1]

If a specialization exists for the class, it will be used. Or the primary template is chosen, which calls the member function.

Thanks, @klshrinidhi. I think this prioritizes the free function over the member though, which is opposite of what we want.

[klshrinidhi]

It chooses the specialization over member function.
I think it makes sense because if I put the effort to specialize the function to change the serialization behavior, I would like it to be called instead of member function. Choosing the member function even when a specialization exists defeats the purpose of specializing. The person who specialized the function will be surprised to see it not take effect.
Again, I am not too familiar with this development. So you may be right.

[chrisdembia]

@klshrinidhi and I just had a whiteboard session and he brought up two other points:

1. Maybe it is desirable to prefer a free function (that is, template specialization of toXmlElement()) over a member function) if both exist. Why would one define the free function if the class already had toXmlElement()/fromXmlElement()? Perhaps the only reason would be because they want to override the behavior of the member function to achieve something new. Probable answer: the control of serializing a type should be held by that type; it would be dangerous for others to subsume that control. The resulting Xml file could not be loaded correctly by others who did not have that free function but were using the same class.
2. Drawing a parallel to std::begin() and std::end(), @klshrinidhi was saying how you can call begin() or end() (without any namespace qualification), and if the argument is in the std namespace then std::begin()/std::end() will be called. He raised the idea that it could be nice to never have to qualify toXmlElement() (or toXmlElementHelper()) with SimTK::. This would require a <ns>::toXmlElement() in each namespace that wanted to use it, though.

[sherm1]

would anything we do here eventually change behavior if Unified Call Syntax becomes part of the standard?

Yikes! Who knows? Some of the proposals would make this easier.

[sherm1]

Moving discussion out of the code ...

[chrisdembia]

Fantastic! I'll start looking at this soon.

[chrisdembia]

Unfortunately, std::is_null_pointer<T> is C++14, not C++11.

[sherm1]

std::is_null_pointer is C++14, not C++11

Removed check for now. Should pass on Linux and OSX now; still one mysterious test failure on Windows.

[sherm1]

I investigated the Windows failure. It appears to be a problem with the Visual Studio linker not correctly generating a single copy of each Value<T> class so that its static members are shared by all. Hence it tries to re-register the deserialization method for Value<double> and triggers an error message. It would be easy to remove the check and ignore subsequent registration (in which case all the tests will pass), but I am worried about what will happen on deserialization because dynamic_cast may not recognize the two Value<double> objects as being the same type. I'll leave it for now as a reminder that further investigation is needed; there may be a workaround that will ensure the linker works properly.

[chrisdembia]

Ah I know very little about that stuff, but would extern help?

[sherm1]

Ah I know very little about that stuff, but would extern help?

Can't be extern because Value<T> is templatized and thus has to be defined in headers. The linker then has a nasty job to perform to ensure that every compilation unit that has instantiated Value<SomeType> for the same template argument uses the same static data members -- I'm depending on that to ensure that each type is registered only once for deserialization.

[sherm1]

Maybe it is desirable to prefer a free function (that is, template specialization of toXmlElement()) over a member function) if both exist.

No, as explained in the Unified Call Syntax doc you linked to, the member function is to be preferred because it was provided by the class designer.

[sherm1]

Drawing a parallel to std::begin() and std::end() ...

Coincidentally that is also discussed in the Unified Call Syntax doc, where it was mostly described as a hack for range-for; it is definitely not a general principle and it didn't seem that they were particularly happy with it. (That's Stroustrup and Sutter so I'm inclined to take them seriously!)

[chrisdembia]

One potential issue with preferring the member function over a free function: say today there is a type X that I want to serialize but I don't have control over it, so I write a free function. Then next year, the author of X decides to add a toXmlElement() member function to that class. Now, the functionality has silently changed.

[sherm1]

Now, the functionality has silently changed.

Hmmm ... any time someone changes the code of a class you are using the behavior can change! Hopefully the author took seriously the need to serialize the class and now you won't have to. Otherwise I recommend sending a nasty email!

[chrisdembia]

I suppose that's true.

@klshrinidhi seemed to be able to get rid of the third argument!

[chrisdembia]

One way to get rid of the name toXmlElementHelper is to rename toXmlElement to toXmlElementImpl, to signify this is the implementation of a function that does the conversion, freeing the use of the name toXmlElement to the interface.

[chrisdembia]

I have started working on a TestStateXml test for Simbody, and will open a PR for that eventually.

Separately, I gave my first shot at writing out an OpenSim Model's State to XML. I got the following error:

26: Test failed due to exception: SimTK Exception thrown at String.h:353:
26:   Error detected by Simbody method String::String(T): Type T=SBModelVars has no stream insertion operator<<(T) and there is no specialized String(T) constructor.
26:   (Required condition '!"no stream insertion operator"' was not met.)

I think this means (1) SBModelVars does not have a toXmlElement() yet, and (2) the error for when toXmlElement() is not implemented for T does not indicate that the issue was a missing toXmlElement(). Actually, I don't understand how this was a run-time error instead of a compile-time one.

[sherm1]

• It's complaining about the stream insertion operator because that is used by default to serialize when there is no toXmlElement() available. Actually it is more contorted than that because it first falls back to writeUnformatted() which is how we get inf and nan to come out right among other things. It is writeUnformatted() that falls back to stream insertion. (Links are to the relevant code.)
• Not sure whether the above contortions are a good idea, but they do save writing a lot of redundant toXmlElement() methods. Better ideas?
• The reason it is a run time error is that there are many uses of Value<T> that don't require serializing and I didn't want to make people provide serialization methods if they don't intend to serialize. The original Value<T> did have this requirement (everyone had to provide operator<<()) and it turned out to be a huge inconvenience and resulted in many indecipherable compilation errors.

[sherm1]

One way to get rid of the name toXmlElementHelper is to rename toXmlElement to toXmlElementImpl, to signify this is the implementation of a function that does the conversion, freeing the use of the name toXmlElement to the interface.

I don't understand. toXmlElement() is the name in the interface at the moment. Please clarify.

[chrisdembia]

Not sure whether the above contortions are a good idea, but they do save writing a lot of redundant toXmlElement() methods. Better ideas?

Thanks for explaining. I like idea of piggybacking off writeUnformatted() and the stream operator. I'm also fine with the run-time message (over a compile-time message). What about just catching such exceptions in the toXmlElement() function that calls writeUnformatted() to provide a more informative message?

I don't understand. toXmlElement() is the name in the interface at the moment. Please clarify.

I mean the interface for the implementor of toXmlElement() methods. I'm thinking of the following: The interface for converting an object to an XML would be SimTK::toXmlElement(obj) (a free method). It may or may not work, depending if obj supports serialization. For some objects that users are more likely to want to serialize, we provide a convenience member function, e.g. State::toXmlElement(). To provide the ability to serialize an object, you either implement toXmlElementImpl(const T&) or T::toXmlElementImpl().

auto xmlDoc = SimTK::toXmlElement(SimTK::State()); // this would work and call SimTK::toXmlElementImpl
Xml::Element SimTK::State::toXmlElement() {
   return this->toXmlElementImpl();
}
Xml::Element SimTK::State::toXmlElementImpl() {
    ...
    elem.appendNode(toXmlElement(...));
    elem.appendNode(toXmlElement(...));
}

Let me know if I'm still not being clear. The gist is that if you want to obtain an Xml::Element, you call toXmlElement(). If you want to provide the ability to convert to Xml, you implement T::toXmlElementImpl(). The fuzzy area is member functions that you call to obtain an Xml::Element, since it's weird for a user to call a method toXmlElementImpl().

[chrisdembia]

Alternatively, we could just rename toXmlElementHelper() to something that more clearly conveys the meaning findSomeWayToConvertThisThingToXmlForMe(). Like convertToXmlElement(), or createXmlElement() or formXmlElement().

[chrisdembia]

I still like the idea of handling the potentially error-prone version and element name code in some central way. Here's a sketch of what I'm thinking:

class XmlSerialization {
public:
    XmlSerialization(std::string type, int version, std::string attrName="");
    template <typename T>
    void appendThing(std::string name, T value);
    Xml::Element getXmlElement() const;
};
...
Xml::Element Measure_Differentiate_Result::toXmlElement(const std::string& name) const {
        static const int version = 1;
        XmlSerialization xml("Measure_Differentiate_Result", version, name);
        xml.appendThing("operand", operand);
        xml.appendThing("operandDot", operandDot);
        xml.appendThing("derivIsGood", derivIsGood);
        return xml.getXmlElement();

I think there are a bunch of flaws with this proposal. For example, how would we require that people use the XmlSerialization class instead of writing their own serialization? Also, I haven't thought through how we would actually handle different versions, or how to combine this with the corresponding fromXmlElement() method. Also, perhaps the decentralized nature of the current serialization scheme is fine (where each class is in charge of doing it correctly). Anyway, it gets rid of some of the boilerplate.

It just sorta feels like the valuable information is "type name", "version", and a list of the members to serialize. It would be good if that were all one needed to specify.

[sherm1]

Well, I'm more confused now! What I want to achieve is Xml::Element = state.toXmlElement() and state.fromXmlElement(element). Those are problematic because States contain AbstractValue objects, whose underlying Value<T> objects can be arbitrary existing types or user-defined types. That requires a user-written method for serializing type T that can be embedded in the serialization of Value<T>, and a corresponding user-written method that can be invoked when deserializing Value<T> objects. These should be member functions when possible (for general object-oriented code reasons), free functions otherwise.

I want those functions to be easy enough to write that there is a high probability of it being done successfully. Since user types will have members of arbitrary type (including other user types) the best way to do this would be recursively. So I'd like to be able to write a T.toXmlElement() method that invokes the TT.toXmlElement() methods of the data members of type TT. But that can't work exactly because we don't know if TT has a member, so I invoked the Helper there to figure out which method to invoke. @klshrinidhi's fix makes that more appealing since it gets rid of the mysterious third argument. It would be perfect I think if it could be named toXmlElement() so that a user writes the T.toXmlElement() method by calling a series of toXmlElement(TT) helper functions which map either to TT's member or a free function toXmlElement(TT). But I don't think that's possible because the helper and the free function would have the same name (is it?).

Anyway I could live with writing the member function or free function toXmlElement() in terms of toXmlElementHelper() with the final argument gone. I guess that's not so bad. Another alternative would to name the member function and helper function toXmlElement() but name the free functions toXmlElementFree() or something.

[chrisdembia]

But I don't think that's possible because the helper and the free function would have the same name (is it?).

Even if it were possible, I think it might be confusing to use the same method name for so many things.

Anyway I could live with writing the member function or free function toXmlElement() in terms of toXmlElementHelper() with the final argument gone. I guess that's not so bad. Another alternative would to name the member function and helper function toXmlElement() but name the free functions toXmlElementFree() or something.

I think either of these are fine.

[chrisdembia]

@sherm1 I am starting to add in the necessary toXmlElement() methods on other types. I was going to start working on SBInstanceVars but I'm not sure I actually know how to serialize this class. While some of the members seem like part of the state (mobilizerLockLevel, constraintIsDisabled), my gut says other members aren't actually part of the state, like bodyMassProperties.

class SBInstanceVars {
public:
    Array_<MassProperties,MobilizedBodyIndex>   bodyMassProperties;
    Array_<Transform,     MobilizedBodyIndex>   outboardMobilizerFrames;
    Array_<Transform,     MobilizedBodyIndex>   inboardMobilizerFrames;

    Array_<Motion::Level, MobilizedBodyIndex>   mobilizerLockLevel;
    Vector                                      lockedQs;
    Vector                                      lockedUs; // also used for udot

    Array_<bool,          MobilizedBodyIndex>   prescribedMotionIsDisabled;

    Vector                                      particleMasses;

    Array_<bool,ConstraintIndex>                constraintIsDisabled;

Could you tell me which of these members should be serialized?

[chrisdembia]

Should this be e.getOptionalAttributeValue("name");? This occurs elsewhere too.

[chrisdembia]

Hmm...after looking at some more of the code, it seems there are two scenarios, one where the name is the tag name, and one where it's the value of the name attribute.

[sherm1]

Yes, I think that's right. That's because I made it optional to create a unique tag for serializing user objects. If the "to" method created a special tag, then the "from" method can look for it. Not sure that was a good idea, but you don't want to have to make up tags for everything (int, char, etc.). They really aren't needed because we always know what type we're expecting by the time we read it in, except for AbstractValue objects where we have the whole registration infrastructure in order to figure out what type we've got.

[sherm1]

(SBInstanceVars) Could you tell me which of these members should be serialized?

It's a variable, so everything in it should be serialized. Body mass properties are instance variables, but there aren't currently any methods for setting them so they just keep their default values.

[chrisdembia]

I'm not sure that solution can work... sometimes, that macro is used inside a class definition, to create a nested type; the free function would end up in the body of the containing class.

[chrisdembia]

Nevermind! You can define the friend function in the class body!

#define SimTK_DEFINE_AND_EXPORT_UNIQUE_LOCAL_INDEX_TYPE \
class EXPORT NAME { ... \
public: \
    friend std::istream& operator>>(std::istream& input, NAME& obj) { input >> obj.ix; }
};

Another question: is there a particular reason why AbstractValue does not have a fromXmlElement(), but instead has a createFromXmlElement()? Is createFromXmlElement() just for testing (it's used in TestXml)?

[sherm1]

is there a particular reason why AbstractValue does not have a fromXmlElement(), but instead has a createFromXmlElement()?

Yes -- it's an abstract class (has pure virtuals) so you can't create one. Only concrete Value<T> objects can be created. So the thing.fromXmlElement(element) form can't work since there can't be a thing until after you've determined (from the element) what kind of concrete object to make.

[chrisdembia]

@sherm1 For testing, I plan to add test cases to TestXml for types in SimTKcommon, and to create a TestSimbodyXml for types in Simbody. To test the ability to fully serialize and deserialize a State object and then use it meaningfully, I was thinking of a TestStateXml for SimTKcommon and a TestSimbodyStateXml for Simbody. Does this sound good?

[sherm1]

I was thinking of a TestStateXml for SimTKcommon and a TestSimbodyStateXml for Simbody

Good idea! TestStateXml can just test the basic functionality while TestSimbodyStateXml checks that we managed to write the (de)serialization methods for all of the junk Simbody puts in the State.

[chrisdembia]

It would be easy to remove the check and ignore subsequent registration (in which case all the tests will pass), but I am worried about what will happen on deserialization because dynamic_cast may not recognize the two Value objects as being the same type. I'll leave it for now as a reminder that further investigation is needed; there may be a workaround that will ensure the linker works properly.

Do you think we could solve this issue now? I'd like to start submitting PRs and it'd be good for the appveyor tests to pass. Let me know how I may be able to help here.

[chrisdembia]

I was trying to learn more about static variables inside functions and came across this blog post that said dynamic initialization of static variables is not threadsafe in some compilers (non-GCC). The blog post is somewhat old, though; this page indicates that statics are threadsafe in MSVC 2015 ("magic statics"). This info doesn't seem relevant to the current issue with MSVC; I just thought it was worth sharing.

[sherm1]

Thread safety of function-local statics was fixed in C++11 so all compliant compilers should behave properly. This stackoverflow answer points to the specification in the standard.

[chrisdembia]

I have a question about if the registration mechanism for Value is sufficient. Is it possible for users to have an executable that deserializes an xml element that contains Value<T>s that are not registered in said executable? For example, OpenSim has an addToSystem() phase in which state and cache variables, etc. are created, and presumably that's when Value<T> objects will be first constructed, and thus registered. However, I may have a generic executable that simply deserializes state objects, independent of Models. In such a case I want to deserialize a state object before (or without) ever invoking Model::initSystem() (which invokes extendAddToSystem() on all components, and thus causes the registration).

We may not readily detect this issue in our tests, because the obvious way to write tests is to first serialize then deserialize. It seems like we need tests that just deserialize from an Xml file that is stored in the repository.

[sherm1]

That's a good point - didn't occur to me. There would have to be a registration phase where AbstractValue::registerValueFactory() gets called for each interesting Value<T>::createFromXmlElement() method. We could do that automatically for known types, maybe on first deserialization call(?). That might even fix the Value<double> problem we've been seeing. There would have to be manual registration for user types T if they are to be deserialized prior to instantiating a Value<T>.

There has to be a State object prior to deserializing. If that has to be a State created by a System many of the Value<T> objects would likely have been instantiated already. But if an empty State can be used it could be the only thing in the program.

[chrisdembia]

If that has to be a State created by a System many of the Value objects would likely have been instantiated already.

This is something I'd like to chat with you about in person at some point. I know you've mentioned before that we might need a System to properly deserialize a State. Right now, if we deserialize a state, about all we can do with it (without a System) is reserialize it. We can't even ask it for its time (b/c it's not realized to Time).

We could do that automatically for known types, maybe on first deserialization call(?).

I'm not sure I understand; how can we register upon deserialization?

[sherm1]

I still like the idea of being able to (de)serialize with only a State around, but I could be talked out of it!

We can't even ask it for its time (b/c it's not realized to Time).

That's not right -- time and state are independent of anything else and don't require realization. If they aren't available something didn't deserialize correctly.

I'm not sure I understand; how can we register upon deserialization?

Someone calls state.fromXmlElement(). Eventually that tries to deserialize a Value. That method checks a flag to see if the bulk registration method has been called; if not it calls it.

[sherm1]

Bulk registration could also be handled with a function static variable similar to what I did for individual registration of Value<T> objects.

[chrisdembia]

Ah bulk registration. I don't know...my hunch is that we should treat all Ts equally. Otherwise, it seems like manual registration will create second-class citizens that won't be well supported.

[chrisdembia]

I still like the idea of being able to (de)serialize with only a State around, but I could be talked out of it!

Yeah I like that idea too.

[chrisdembia]

Things to discuss:

1. Naming for toXmlElementHelper/fromXmlElementHelper. The issue with naming the free function toXmlElementFree() is that it is then too easy to subvert the desired preference of member function over free function by an unsavory user who just specializes toXmlElement() instead. dispatcher.
2. How to register Value's, considering that they may not have been constructed in the program trying to deserialize.
3. If we like fromXmlElementHelperHelper, what should we name, it? no get rid of it.
4. Should there be a corresponding toXmlElementHelperHelper. no
5. Deserializing types that must obey certain restrictions (UnitVec3, Rotation), and the tolerance for obeying the restrictions. (I am also struggling with implementing fromXmlElement for UnitVec3.
6. Do we need a System to deserialize a State? Right now, OpenSim tries to validate a StatesTrajectory, which requires calling getTime() and isConsistent(), etc. Opinion about OpenSim classes: StatesTrajectory has to/from Xml, but then another class "Simulation" combines the StatesTrajectory and a Model into a file.
7. Wrapping another Xml Element around the writeUnformatted specialization (for consistency).
8. Symmetry of Inertia tensor does not work with Infinity (the comparison thinks Infinity != Infinity).
9. Fixing the MSVC static local issue.
10. The necessity of void_t. OK. common.h. reference the docs.
11. OpenSim's 5th Subsystem is just "Subsystem."

[sherm1]

@chrisdembia: Ah I know very little about that stuff, but would extern help?

This was from way up this thread, referring to the problem where different instantiations of Value<double> got made in different compilation units causes a dynamic_cast failure. Now I think that extern template class Value<double>; in a header with an explicit Value<double> instantiation in a .cpp might successfully prevent the duplication.

I know this is paused at the moment but a similar problem came up in Drake so I thought I'd record that thought here.

[chrisdembia]

Ah...interesting. I will try that out whenever I get back to this project. Thank you for sharing.