I've been perusing the files you checked, your example, and this list. Summary ======= First of all, a little more complete narrative description as to what the submission was intended to acommplish and how it would change the way the user uses the library would have been helpful. I'm going to summarize here what I think I understand about this. Please correct me if I get something wrong. a) a new trait is created. template <class Archive, class Type> struct has_fast_array_serialization : public mpl::bool_<false> {}; b) new functions save_array and load_array are implemented in those archives which have the above trait set to true. In this case the following is added to the binary_iarchive.hpp file. The effect is that this trait will return true when a fundamental type is to be saved/loaded to a binary_iarchive. // specialize has_fast_array_serialization // the binary archive provides fast array serialization for all fundamental types template <class Type> struct has_fast_array_serialization<binary_iarchive,Type> : public is_fundamental<Type> {}; c) A user with the following class my_class { valarray<int> m_a; template<class Archive> void save(Archive & ar, const unsigned int){ ar << m_a } ... }; d) In order for this to work, data types which want to exploit bitwise serialization have to look like the following - taken from valarray - serialization. This would have to be applied to serialization template<class Archive, class U> inline boost::disable_if<boost::archive::has_fast_array_serialization<Archive,U>
::type save( Archive & ar, const STD::valarray<U> &t, const unsigned int /* file_version */) { const boost::archive::container_size_type count(t.size()); ar << BOOST_SERIALIZATION_NVP(count); for (std::size_t i=0; i<t.size();+i) ar << t[i]; }
// with fast array serialization template<class Archive, class U> inline boost::enable_if<boost::archive::has_fast_array_serialization<Archive,U>
::type save( Archive & ar, const STD::valarray<U> &t, const unsigned int /* file_version */) { const boost::archive::container_size_type count(t.size()); ar << BOOST_SERIALIZATION_NVP(count); if (count) ar.save_array(boost::detail::get_data(t),t.size()); }
Presumably this treatment would be applied to: std::vector ublas::vector ublas::matrix mtl::vector blitz::array custom_lib::fast_matrix ... as well as others. For built-in arrays, the core library is tweaked to include code similar to the above for built-in arrays. Some Observations ================= Inmediatly the following come to mind. a) I'm not sure about the portability of enable_if. Would this not break the whole serialization system for those compilers which don't support it? b) what is the the point of save_array? why not just invoke save_binary directly? c) The same could be said for built arrays - just invoke save_binary d) There is no provision for NVP in the non-binary version above while in the binary version there is NVP around count. Presumably, these are oversights. e) The whole thing isn't obvious and its hard to follow. It couples the implementation code in i/o serializer.hpp to a specific kind of archive adding another dimension to be considered while understanding this thing. f) What about bitwise serializble types which aren't fundamental? That is structures which don't have things like pointers in them. They have the same opportunity but aren't addressed. If this is a good idea for fundamental types, someone is going to want to do them as well - which would open up some new problems. g) I don't see endian-ness addressed anywhere. I believe that protocols such as XDR and MPI are designed to transmit binary data between heterogenious machines. Suppose I save an array of ints as a sequence of raw bits on an intel type machine. Then I use load_binary to reload the same seqence of bits into sparc based machine. I won't get back the same data values. So either either the method will have to be limited to collections of bytes or some extra machinery would have to be added to conditionally to the endian translation depending on the source/target machine match/mismatch. f) Similar issues confront bitwise serialization of floats and doubles. I believe the "canonical" format for floats/doubles is ieee 80 bit. (I think that's what XDR uses - I could be wrong.) I believe that many machines store floats as 32 bit word and doubles as 64 bit words. I doubt they all are guarenteed to have the same format as far as exponent, sign and representation of value. So that's something else to be addressed. Of course endian-ness plays into this as well. g) I looked at the "benchmark" results. I notice that they are run with -O2 on the gcc compiler. Documentation for the gcc compiler command line specifies that this optimization level should does not enable automatic inlining for small functions. This is a crucial optimization to be effective in the serialization library. The library is written with the view that compilers will collapse inline code when possible. But this happens only in the gcc compiler when the -O3 optimization switch is used. Furthermore, with this compiler, it might be necessary to also specify max-inline-insns-recursive-auto switch. to gain maximum performance on boost type code. This latter is still under investigation. h) my own rudimentary benchmark (which was posted on this list) used 1000 instances of a structure which contained all C++ primitive data types plus an std::string made up of random characters. It was compiled as a boost test and built with bjam so it used the standard boost options for release mode. It compared timings against using raw stream i/o. Timings for binary_archive and standard stream i/o where comparable. I'm still working on this test. The problem is that standard stream i/o uses text output/input. Of course no one for whom performance is an issue would do this so I have to alter my timing test to use binary i/o to the standard stream as a comparison. But for now, I'm comfortable in asserting that there is not a large performance penalty using serialization as opposed to "rolling your own". As an aside, the test executable doing the same test for 3 different types of archives and all primitive data types only came to 238K. So there isn't a significant code bloat issue either. i) somehow I doubt that this archive type has been tested with all the serialization test suite. Instructions for doing so are in the documenation and the serialization/test directory includes batch files for doing this with one's own archives. Was this done? What where the results? With which compiler? It costs nothing to do this. end of observations =================== Admitedly, this is only a cursory examination. But its more than enough to make me skeptical of the whole idea. I you want, I could expand upon my reasons for this view, but I think they should be obvious. Now if someone feels differently and wants to implement such a thing, they have my full support. There is no need to modify the core library and no benefit - performance or otherwise. The following shows how to go about this. For purposes of this exposition, I am going to limit myself to how one would go about crafting a system similar to the one submitted. That is, I will not address concerns such as binary portability as the are not addressed in the submission as I see it. I'm assuming that the only issue is how best to arrange things so that save_binary/load_binary are invoked in for contiguous collections of fundamental types. Suggestions =========== I do see the utility and merit in what you're trying to do here - finally. Honestly it just wasn't obvious from the initial package. So here is how I would have gone about it. I previously posted a demo "fast_oarchive.hpp" I will expand upon it here. the archive class tree would look like the following (setting aside polymorphic archives). I would envision the following class basic_archive basic_oarchive common_oarchive basic_binary_oarchive binary_oarchive fast_oarchive_impl MPI_oarchive XDR_oarchive ... fast_oarchive_impl is an adaptor which can be applied to any legal archive. The current submission derives from binary_?archive. If this is all that is required then it doesn't have to be a template, it could just as well be derived directly from binary_oarchive. It includes overloads for the following: template<class T> void save_array(T & t, collection_size_t count){ // this version not certified for more complex types !!! BOOST_STATIC_ASSERT(boost::is_primitive<T>::value); // or pointers either !!! BOOST_STATIC_ASSERT(boost::is_pointer<T>::value); ... // if T is NOT a fundamental type - some mpl magic required here // foward call to base class this->Base::save_override(t, 0); // else - *(this->This()) << make_nvp("count", t.size() * sizeof(T)); *(this->This()) << make_nvp(make_binary_object(t.size() * sizeof(T), & t)); // note - the nvp wrappers probably not necessary of we're only // only going to apply this to binary archives. } // here's a way to do it for all vectors in one shot template<class T> void save_override(const std::vector<T> & t, int){ save_array(t, t.size()); } ... boost::valarray? ... // for C++ built-in arrays template<typename T, int N> void save_override(const T (& t)[N] , int){ save_array(t, sizeof(t)); } It might or might not contain similar implementations for std::vector ublas::vector ublas::matrix mtl::vector blitz::array custom_lib::fast_matrix etc For now assume that it does - later we'll relax that assumption. derived from fast_oarchive_impl are your archive classes for specific archive types - MPI_oarchive, .... These would handle the specific features of each particular archive. If it turns out that MPI can only apply the save_binary optimization to types that are no more than a character wide (perhaps for endien issues) then it would include something like: template<class T> void save_override(const std::vector<T> & t, int){ // suppose endian issues preclude save_binary preclude if(sizeof(T) > 1){ // skip over save_binary optimization this->binary_oarchive::save_override(t, 0) } else // just forward to the base class this->fast_oarchive_impl::save_override(t, 0) } So net result is: a) save_binary optimizations are invoked from the fast_oarchive_impl class. They only have to be specified once even though they are used in more than one variation of the binary archive. That is, if there are N types to be subjected to the treatment by M archives - there are only N overrides - regardless of the size of M. b) save_binary optimizations can be overriden for any particular archive types. (It's not clear to me how the current submission would address such a situation). c) There is no need alter the current library. d) It doesn't require that anything in the current library be conditioned on what kind of archive is being used. Insertion of such a coupling would be extremely unfortunate and create a lot of future maintainence work. This would be extremely unfortunate for such a special purpose library. This is especially true since its absolutly unnecessary. e) The implemenation above, could easily be improved to be resolved totally at compile time. Built with a high quality compiler (with the appropriate optimization switches set), this would result in fastest possible code. f) all code for save_binary is in one place - within fast_oarchive_impl. If fast_oarchive_impl is implemented as a template, it could be applied to any existing archive class - even text and xml. I don't know if there would be any interest in doing that - but it's not inconcievable. Note also that including all the save_binary optimizations in for all of std::vector ublas::vector ublas::matrix mtl::vector blitz::array custom_lib::fast_matrix doesn't require that the headers for these libraries be included. The code in the header isn't required until the template is instantiated. So there wouldn't be any "header bloat"(tm) g) Now, f above could also be seen as a disadvantage. That is, it might seem better to let each one involved in serialization of a particular collection keep his stuff separate. There are a couple of options here I will sketch out. i) one could make a new trait is_bitwise_serializable whose default value is false. For each collection type one would specialize this like: template<class T> struct is_bitwise_serializable<vector <T> > { ... is_fundamental<T> ... get_size(){ // override default options which is sizeof(T) .. } Now fast_oarchive_impl would contain something like: // here's a way to do it for all vectors in one shot template<class T> void save_override(const T & t, int){ // if T is NOT bitwise serializable - insert mpl magic required here // foward call to base class this->Base::save_override(t, 0); // else - *(this->This()) << make_nvp("count", t.size() * sizeof(T)); *(this->This()) << make_nvp(make_binary_object(...get_size(), & t)); // note - the nvp wrappers probably not necessary of we're only // only going to apply this to binary archives. } Which would implement the save_binary optimization for all types with the the is_bitwise_serializable trait set. Of course any class derived from fast_oarchive_impl could override this as before. Note that this would address the situation whereby one has something like struct RGB { unsigned char red; unsigned char green; unsigned char blue; }; which certainly would be candidate for save_binary optimization - it would even be portable across machines of incompatible machines - but then there might be alignment issues. Also, if someone tracks an object of type RGB somewhere, then this would work differently on different archives - not a good thing. So this would require some careful documentation on how to use such a trait. This would move the information about save_binary optimization out of the fast_oarchive_impl.hpp header file and into the header file for each particular type - arguable a better choice. ii) another option would be to implement differing serializations depending upon the archive type. So that we might have template<class T> void save(fast_oarchive_impl &ar, const std::vector<T> & t, const unsigned int){ // if T is a fundamental type or .... ar << t.size(); ar.save_binary(t.size() * sizeof(T), t.data?()); } This would basically much simpler substitute for the "fast_archive_trait" proposed by the submission. Wrap up ======= I guess it should be pretty obvious that I believe that a) The applicability and utility of a save/load binary optimization are narrow than claimed. b) The claims of performance improvement are suspect. c) This implementation doesn't address all the issues that need to be addressed for something like this. d) Such an idea could be implemented in a much simpler, more transparent, robust, and efficient manner. e) Such "special" implementation features are easily accommodated by the current library by extension. There is no need to change any of the core library implementation or interface. Finally, I have to comment on the way this has been proposed. Checking a diverse group into the CVS system on a separate branch is not convenient for most people. This would better be a zip file uploaded to the vault. It should also include some sort of document outlining what its intended to do and how it does it. Had this been done, its likely that many more people would have had a look at it and been able to commment. I'm sure the issues i've noted above would be apparent to a lot of people and I wouldn't have had to spend a few hours preparing this critique. If the above weren't bad enough, I'm pretty much been forced to do what really amounts to doing a private consulting job for free for a specific application. I really can't do this anymore. Its one think to point a user in the right direction (which often results in tweak to the manual) or incorporate a bug fix - which some user has tracked down, fixed and submitted. But to have to spend the time to critique something like this - whose problems should be obvious to most of us. is really unfair. I've been through it a couple of times - I'm not doing it again. If you feel that you need to split off the serialization library to do what you want, I've a better idea: How about you and Matthias taking it over yourself? After all, I've achieved all I expect to get from it. Then you would be free to make any changes you want without wasting time on these discussions. Of course if the problems in the above are only obvious to me, then it probably would indicate that I'm out of sync with the boost development community - in which case it really should be taken over by someone else. Robert Ramey