[boost] array optimized serialization (was: [MPI] Review comments)

From: Matthias Troyer (troyer_at_[hidden])
Date: 2006-09-19 06:01:57

Hi Robert,

Since this is a discussion orthogonal to the MPI review, I have  
renamed the subject.

On Sep 19, 2006, at 12:30 AM, Robert Ramey wrote:

> Matthias Troyer wrote:
>> On Sep 18, 2006, at 8:50 PM, Robert Ramey wrote:
>
>> Thus, there is no std::vector, std::valarray, ... overload in any of
>> the archives - not in the binary archive nor anywhere else.
>
> Well, we're not seeing the same thing.
>
> template<class Archive>
> class basic_binary_oarchive :
>     public array::oarchive<Archive>
> {
> ...
>
> template <class Archive>
> class oarchive
>  : public archive::detail::common_oarchive<Archive>
>
>  [snip - implementation details - snip]

OK, now I understand your point. Let us recall the original design  
that we had discussed and that was in the end your suggestion:

1. introduce an array wrapper array<ValueType> to be used in the  
serialize function of all classes that can profit from array  
optimizations

2. provide a default serialization for the array wrapper

3. overload the array< ValueType>  serialization in those classes  
that can optimize array serialization

4. to simplify these optimizations and avoid code duplication provide  
a wrapper like

   template <class Archive> array_oarchive<Archive> : Archive {....}

   to "add" array optimization to existing archives

I hope you do not want to change this design now?

We had next implemented just this, and placed in a separate namespace  
archive::array, without touching your binary archive. The  
save_override for array<ValueType> with all the dispatch logic you  
quoted above was implemented in this wrapper.

In addition, we provided, in the archive::array namespace, separated  
from yours, an array optimized binary archive using that wrapper.  
Then, on June 4th you sent me an e-mail, suggesting that I move the  
array<ValueType> optimization into your binary archive. I quote from  
your e-mail:

#Howevever, once we introduce the concept of wrapper, we can  
introduce the
# special wrapper type into binary_archive without changing the  
requirements
# for other archives. This is the model for xml_?archives.  So I  
think you
# should move your array type to somewhere higher - perhaps as
# basic_binary_?archive.  This would eliminate a set of special  
classes just
# for this.  In this way we all get what we want: You get enhanced
# binary_?archive - I keep the minimal set of requirements for all new
# archives.

The easiest way of achieving this without code duplication was to  
actually provide an array-optimized base class from which all  
archives using array optimization could derive, and to put all of the  
array optimization logic into this base class array::oarchive, from  
which you quoted above.

It seems now, however, that you dislike this, and that's why I  
proposed to just roll back to the state before June 4th, thus not  
touching any of your archives, and again providing our own version of  
an array optimized binary archive. You can then implement the array  
optimization as you feel like for your archives, and we have our  
mechanism for our archives. Please let me know if I should do this.

> which just moves some of the implementation out of binary_oarchive
> into oarchive.  What attracts my attention is:
>
>   // dispatch saving of arrays to the optimized version where  
> supported
>   template<class ValueType>
>   void save_override(serialization::array<ValueType> const& x,  
> unsigned int
> version)
>   {
>
> Now these suggest to me that the next person who want's to handle his
> wrapper
> specially for certain archives will then want/have to back into
> binary_oarchive
> and/or oarchive to add special handling to HIS wrapper.  That is  
> what I mean
> by not scalable.

Actually, as you can see from your own mail of June 4th, it was *you*  
who suggested to move this save_override into the binary archive. If  
you now have doubts about this idea of yours, then we can again roll  
back to the CVS state of June 4th.

>   template<class ValueType, class Allocator>
>   void save_override(std::vector<ValueType,Allocator> const &x,  
> unsigned int
> version)
>   {
>
> And then there is the special handling for std::vector - which  
> suggests that
> when
> one want's to add special handling for multi_array he'll have to do
> something else.

No, this will not be needed for multi_array, nor for ublas or Blitz  
arrays or any other array class which requires default constructible  
value types. The std::vector override is only a workaround until we  
have an is_default_constructible traits in Boost. If your main  
objection is to that overload, then we can just implement  
is_default_constructible and remove that workaround.

> Finally - the usage of inheritance used above strikes me as confusing,
> misleading, and unnecessary.  This may or may not be strictly an
> aesthetic issue.  The only really public/protected function is the
> overload for the array wrapper and the default forwarder. These
> entry point functions are only called explictly by the drived class.
> Basically the base class is not being used to express an "IS-A"
> relationship but rather an "Implemented in terms of" relationship
> as decribed in Scott Meyers Effective C++ item 41. So I would
> have expected something in binary_oarchive like
>
> template<class T>
> void save_override(array<T> & t, const unsigned int version) const {
>     // if array is optmizable
>     save_optimized_array(t, version); // most of oarray in here
>    // else
>    // forward to default handling
> }
>
> This would have to be added to every archive which supports this
> but then we now have to explictly forward twice anyhow so I don't
> see a hugh difference.

Well, you are suggesting to just copy this same function plus the  
"  // if array is optmizable " dispatch logic into every new archive  
class that supports array optimization. Isn't heritance just the  
mechanism to avoid this copy&pasting of hundred lines of identical  
code? Again, as I said before, if you do not like this in your binary  
archive, we can easily remove it from there again and you can do your  
own implementation, while we do ours. Sha

>
>> What you
>> seem to propose, both above and in the longer text I cut, is to
>> instead re-implement the optimized serialization for all these N
>> classes in the M different archive types that can use it (we have M=4
>> now with the binary, packed MPI, MPI datatype, and skeleton archives,
>> and soon we'll do M+=2 by adding XDR and HDF5 archives.). Besides
>> leading to an M*N problem, which the array wrapper was designed to
>> solve, this leads to intrusion problems into all classes that need to
>> be serialized (including multi_array and all others), which is not
>> feasible as we discussed last year.
>
> I am aware of the desire to avoid the M*N problem - that should be
> pretty clear from the design of the serialization library itself -  
> which
> takes great pains to permit any serializable type to be saved/load
> from any archive.  The problem is that when I look at the code, its
> not clear that this problem is being addressed.
>
> And its not even clear that there is an M*N problem here:
>
> binary archives benefit from optimization of types which are
> "binary serializable"
>
> mpi_archives benefit from optimizaton of types for which there
> exists a predefined MPI prmitive.
>
> etc.
>
> There may or may not be overlapp here.  But it seems to me
> that we're trying to hard to factor where truely common
> factors don't exist.
>
> So I'm looking at
> binary, packed - 3 archives - valarray, vector, C++ native arrays 3  
> types- 9
> overloads
> and one special trait - is_binary_serializable

No, wait a moment!

If by packed you mean a packed MPI archive then  
is_binary_serializable is not the right trait.

Also, where do you get only 3 types? We will have multi_array, ublas  
matrices and vectors, MTL arrays, Blitz arrays and many more coming.  
But the array wrapper reduced this down to just one class: array.

> mpi - 1 archive * 3 types - 3 overloads
> and one special trait - is_mpi_optimizable

Again why 3 overloads? There is only one for array (and the temporary  
workaround for vector which we can remove if you want).

So, if we keep the array wrapper then we are halfways there, Having  
reduced the M*N to an M*1 problem. What we have done next is to go  
further and simplify even this M*1 problem  by introducing a base  
class that implements the dispatch logic for array. I believe that  
this is what you do not like. Thus we can easily remove it again from  
your archive, and leave the array optimization for your binary  
archive to you, while we use our method for the archives that we are  
writing (packed MPI, MPI datatype, skeleton, XDR, HDF5).

> Hmmm - when I proposed it I had in mind that it would be used
> differently - as I outlined above.  This is not anyone's fault, it  
> just
> didn't occur to me that it would occur to anyone to use it  
> differently.
>
> And truth is, I've really just wanted to keep ehancements/extensions
> to the library orthogonal to the original concepts.  I thought the
> array wrapper suggestion would accomplish that so once it was
> "accepted" I got back to may paying job.

OK, since I also do not get paid for this, and your criticism is  
based on a change that you yourself proposed on June 4th, my proposal  
is to stop the discussion right here, go back to the state of June  
4th, where our optimizations were completely decoupled from your  
archives. That way you will be able to implement the array  
optimization for the binary archive in the way you like best, and we  
have our own way. Please confirm and I will do that.

Matthias