[boost] [Review] Boost.Endian

Subject: [boost] [Review] Boost.Endian
From: john filo (filo_at_[hidden])
Date: 2011-09-07 13:03:17

- What is your evaluation of the design?

For the original scope (i.e. integers), very good.  However, I'd
like to see additional support for float, double, and user defined
types added.  Without it, this library isn't very useful to me
personally since a large percentage of the values that I have to
deal with are either float or double.

----------------------------------------------------------------------
- What is your evaluation of the documentation?

Excellent.

Is a tutorial section going to be added?  I'm not sure one is needed
given the existing examples in the rest of the documentation, but the
documentation's menu bar implies there will be one.

----------------------------------------------------------------------
- What is your evaluation of the implementation?

Very good.

At -O2, I was impressed with the performance relative to GCC's
builtin byteswap routines.  When I first saw the implementation, I
honestly didn't think it would perform very well given its complexity
compared to a naive implementation (e.g. the version used in the
endian::reorder functions).  I was pleasantly surprised.  I guess
that's a testament to the level of optimization compilers are capable
of these days.

On an Intel Core2, the performance penalty of the uint32 endian<>
template and the reorder() function relative to GCC-4.4 builtins
(i.e. runtime divided by runtime of builtin) is as follows:

      endian<>  reorder()
------------------------
-O0    5.4       3.0
-O2    2.3       6.5

It seems like the reorder functions could be implemented in terms of
the integer types for improved performance (and also maintainability).
Something like:

template<typename T>
inline void reorder(T& x)
{
     x = *reinterpret_cast<
         endian<endianness::nonnative, T, sizeof(T)/8> *>(&x);
}

template<typename T>
inline void reorder(T source, T& target)
{
     target = *reinterpret_cast<
         endian<endianness::nonnative, T, sizeof(T)/8> *>(&source);
}

Obviously the above requires extending the endianess enum with
a "nonnative" value.

Performance isn't that critical for me, but it does seem like
providing platform specific implementations that take advantage of
things like GCC's builtin byteswap functions should also be considered
to get the performance up even further.

Finally, for my last performance related comment, I was surprised to
see that the native types had basically exact same performance as the
nonnative types.  Couldn't the native types be specialized to
effectively be a no-op?

Would "byteswap" or maybe "swapbytes" be a better name than "reorder"?

----------------------------------------------------------------------
- What is your evaluation of the potential usefulness of the library?

For those who deal with non-native endian data, this library is
extremely useful.  It automatically eliminates a whole class of common
programming errors when dealing with such data.

----------------------------------------------------------------------
- Did you try to use the library?  With what compiler?

Yes; gcc-4.4.3 on Linux

----------------------------------------------------------------------
- Are you knowledgeable about the problem domain?

Yes.  I've used a similar home-grown library for the past 10 years,
and have been looking forward this being added to Boost for a long
time.

----------------------------------------------------------------------
 > - Do you think the library should be accepted as a Boost library?

Absolutely.  I'd like to see support for float and double, but
even without those additions, I still vote yes.