> On April 15, 2015, 3:04 a.m., Andrew Stitcher wrote:
> > I still really don't like the code changing depending on the endianness of
> > the platform - Wouldn't it just be better to read the value as either a 4
> > or 8 byte int (uint32_t or uint64_t) and then just use a union to read the
> > value as float or double?
> > ie
> >
> > union {double d; uint64_t i} converter64;
> > union {float f; uint32_t i} converter32;
> >
> > converter64.i = bytevalue;
> > return converter64.d;
> >
> > etc.
> >
> > - It's a bit of a hack but it is pretty portable as the alignment is
> > required by the standard (I'm pretty sure)
> >
> > Then we can just get rid of copyConvert & convertIfRequired which would
> > make me happier.
>
> Alan Conway wrote:
> I don't mind how we do the byte swapping as long as we only do it in one
> place. The current situation where the shift logic is spread all over and we
> mix in the Endian stuff is bad. I'll clean it up.
> I don't understand your attachment to shifting. The goal is to get
> unaligned network-order bytes into an aligned machine-order location. I did
> some tests and the Endian for loop is a terrible way to do it (much worse
> than shifting) but on linux byteswap.h and endian.h are about twice as fast
> as shifting and Visual C++ also has fast byte-swap built-ins. I'm leaning
> towards a portable shift implementation replaced by endian.h if available.
The point is that there is no byte swapping per-se there is just the need to
decode bytes on the wire to floats (and the reverse).
My "attachment" to shifting is purely that it correctly represents the
mathematics of the decode/encode without recourse to the irrelevant and non
portable concept of endianness.
Given that the IEE-754 1985 standard is what AMQP uses for float encoding on
the wire the simplest way to decode is to turn on the wire representation to an
int and then reinterpret as a float (this assumes that the endianness of ints
and floats is the same on every platform, but the current code does this as
well).
Are you saying that the performance of this operation is significant overall to
the performance of the broker? I assume that you did some micro-benchmarks
above.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33194/#review80147
-----------------------------------------------------------
On April 15, 2015, 9:24 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33194/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 9:24 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Previous code would incorrectly convert between float and int types producing
> nonsense values,
> and would not allow legal conversions between float and double types.
>
> Created FixedWidthIntValue and FixedWidthFloatValue template subclasses to
> correctly
> handle conversions. Enabled FieldValue unit tests for float conversions.
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/CMakeLists.txt 1673017
> trunk/qpid/cpp/src/qpid/framing/Endian.h 1673017
> trunk/qpid/cpp/src/qpid/framing/Endian.cpp 1673017
> trunk/qpid/cpp/src/qpid/framing/FieldTable.cpp 1673017
> trunk/qpid/cpp/src/qpid/framing/FieldValue.h 1673017
> trunk/qpid/cpp/src/qpid/framing/FieldValue.cpp 1673017
> trunk/qpid/cpp/src/tests/FieldTable.cpp 1673017
> trunk/qpid/cpp/src/tests/FieldValue.cpp 1673017
>
> Diff: https://reviews.apache.org/r/33194/diff/
>
>
> Testing
> -------
>
> Added unit tests for float conversions.
>
>
> Thanks,
>
> Alan Conway
>
>
