Hi Robert On Sep 17, 2006, at 7:56 PM, Robert Ramey wrote:
This isn't a full review - I've just read the documentation and perused the code. Its more like random observations. A lot of this is very particular to the way that the MPI library uses/builds upon serialization. So it may not be of interested to many.
I am a bit perplexed by your mail, since it is an identical copy of a private e-mail you sent me two weeks ago, even before the review started. I had replied to your e-mail but never heard back from you. I am thus confused about the purpose of your message but assume that you want me to reply in public now. I will essentially follow what I have written in my reply two weeks ago but modify it a bit so that the list readers who have not followed our private e-mail exchange can follow.
I've spent some more time reviewing the serialization currently checked into the head. It's quite hard to follow. Among other things, the MPI library includes the following:
a) an optimization for serialization of std::vector, std::valarray and native C++ arrays in binary archives.
I assume you mean the array wrapper here? If yes, then this is not restricted to the above types but also needed for other array-like data structures. Furthermore this is not part of the MPI library but a general extension to the serialization library that we had implemented there last spring, with your consent. The comments of several reviewers, which were initially skeptical about our use of the serialization library in a high performance context, but whose concerns vanished when they saw the array optimizations, should show you that it was not only me who needs these optimizations.
b) A new type of archive (which should be called mpi_?archive) which serializes C++ structures in terms of MPI datatypes. This would complement the archive types that are already included in the package.
Do you mean the mpi::packed_archive We prefer to call it mpi::packed_archive, since it is actually a slow and not the preferred way of sending via MPI. This does not make use of MPI data types, and is in fact - as you saw yourself - a variant of the binary_archive, with the only main distinction that save_binary cannot be used for fast serialization of array wrappers - a different function is needed, hence the abstraction using save_array.
i) text - renders C++ structures in terms of a long string of characters - the simplest portable method. ii) binary - renders C++ structures as native binary data. The fastest method - but non portable. iii) renders ... as xml elements - a special case of i) above.
So we would end up with an mpi_archive and optionally mpi_primitive. In the other archives, I separated ?_primitive so this could be shared by both text and xml. In your case it isn't necessary to make an mpi_primitive class - though it might be helpful and it would certainly be convenient to leverage on the established pattern to ease understanding for the casual reader.
Actually we already have an MPI primitive class, and the packed archive is - as you saw - just a binary archive with special primitives, and a special way of dealing with arrays. if we keep a save_array instead of just using save_binary in the binary archives, then indeed just by specifying the MPI primitives we create an MPI archive - I liked that design of yours once I grasped how you split off the primitives. Note, however, as Doug Gregor pointed out, that the mpi_primitives are not very useful outside an MPI message passing context.
c) the "skeleton" idea - which I still haven't totally figured out yet. I believe I would characterize this as an "archive adaptor" which changes the behavior of any the archive class to which it is applied. In this way it is similar to the "polymorphic_?archive" .
Indeed these are similar wrappers
In my view these enhancements are each independent of one another. This is not reflected in the current implementation.
Have you looked at the code? Actually the skeleton "archive adaptors" do not depend at all on the rest of the MPI library and they can easily be factored out in case they are useful also in another context. For now, since they are an implementation detail, never occur in the public API, and we do not see them useful in another context at the moment, we have left them in detail.
I would suggest the following:
a) enhancements to the binary archive be handled as such. We're only talking about specializations for three templates - std::vector, std:valarray and native C++ arrays. I know these same three are also handled specially for mpi_?archives, but it's still a mistake to combine them. in binary_? archive they are handled one (load binary) while in mpi_archive they are handled another (load_array) I still think this would be best implemented as "enhanced_binary_?archive".
Watch out that there are more such types: multi_array, ublas and MTL vectors and matrices, ... With the array wrapper we have an elegant solution to handle also these other types. Since we have discussed this topic many times on the list over the past year I will not comment further for now. If you do not like the way we have implemented the array optimizations in the binary archive then we can just roll back the CVS state to the version at the end of May where we had implemented a separate array-optimized binary archive, and non of the MPI archives needed to change any of your archives.
b) mpi_?archive should derive directly from common_?archive like basic_binary_?archive does. The reason I have basic_... is that for xml and text there are separate wide character versions so I wanted to factor out the commonality. In your case, I don't think that's necessary so I would expect your hierarchy would look like class mpi_archive : public common_archive, public interface_archive ...
Do you mean the packed archive? This is actually a binary archive - do you really mean that we should reimplement the functionality of the binary archive and not reuse what is there?
Note that you've used packed_archive - I would use mpi_archive instead. I think this is a better description of what it is.
I still prefer mpi::packed_archive, since there can also be other MPI archives. One possible addition to speed up things on homogeneous machines might be just an mpi::binary_archive, using a binary buffer.
Really its only a name change - and "packed archive" is already inside an mpi namespace so its not a huge issue. BUT I'm wondering if the idea of rendering C++ data structures as MPI primitives should be more orthogonal to MPI prototcol itself. That is, might it not be sometimes convenient to save such serializations to disk? Wouldn' this provide a portable binary format for free? (Lots of people have asked for this but no one as been sufficiently interested to actually invest the required effort).
As Doug Gregor pointed out this is not possible since the format is implementation-defined, and can change from one execution to another.
4) Shouldn't there be a logical place for other archive types for message passing - how about XDR? I would think it would be close cousin to MPI archives.
XDR might be used by an implementation or not - these are implementation details and a good MPI implementation is supposed to pick the best format.
c) The skeleton idea would be template<class BaseArchive> class skeleton_archive ....??? (I concede I haven't studied this enough).
Indeed, the skeleton archives could be factored out if anybody sees another use for them. This is an orthogonal piece of code, and we should discuss where it can be useful. One possible application is to visualize data structures without caring about the content, but only about types and pointers. But I don't know if anyone needs this or if there is another use for these code pieces. If there is then we can factor it out of the mpi detail namespace and put it into archive with no essential changes to the code.
The only "repeated" or shared code might be that which determines when either a binary or mpi optimization can be applied. It's not clear to me whether this criteria applies to both kinds of archives ore each one has its own separate criteria. If it's the latter - there's no shared code and we're done. If it's the former, the a separate free standing concept has to be invented. In the past I've called this "binary serializable" and more lately "magic". ( a concession to physicist's fondness for whimsical names).
The set of types for which an array optimization can be done is different for binary, MPI, XDR, ... archives, but a common dispatch mechanism is possible, which is what we have implemented in the array::[io]archive classes. Your "magic" idea (which you have not described to the list yet since it was only in private e-mails) can easily be incorporated into that. Just replace typedef is_fundamental<mpl::_1> use_array_optimization; by typedef is_bitwise_serializable<mpl::_1> use_array_optimization; or typedef is_magic<mpl::_1> use_array_optimization; and you have upgraded to your magic optimization!
So rather or in addtion to an MPI library you would end up with three logically distinct things. Each one can stand on its own.
So depending on this last, the serialization part of the MPI library falls into 3 or 4 independent pieces. If the code where shuffled around to reflect this, it would be much easier to use, test, verify, enhance and understand. Also the skeleton concept might be then applicable to other types of archives. Also the "magic" concept really is a feature of the type and is really part of the ad hoc C++ type reflection which is what serialization traits are.
If by three or four logically distinct things you mean 1. the array optimization 2. the skeleton&content archive wrappers 3. the MPI archives 4. the MPI library then my comments are: 1. is already factored out and in the serialization library. If anything should be done to it, there was the desire to extend array wrappers to strided arrays, which can easily be done without touching anything in the serialization library. 2. is independent of the rest of the proposed Boost.MPI library but we keep it in detail since we do not see any other use for this at the moment. Once someone could use it we can move it immediately to the serialization library. 3. and 4. are tightly coupled since the MPI archives do not make any sense outside the Boost.MPI context and I do not see that splitting this into two separate libraries makes any sense at all. The code itself is written cleanly though, with no part of the MPI archive types depending on any of the communication functions. Thus I see absolutely no reason at all to shuffle the code around anymore, unless you can come up with a reason to move the implementation details of skeleton&content to a public place in the serialization library. Matthias