> 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.
>
> Andrew Stitcher wrote:
> 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.
Yep micro benchmarks (I'll send you), no idea about impact on broker
performance. I've put in a shifting solution so you can be happy :) I'm not
going to bother with anything else for qpid - if I get time I might do some
more serious benchmarks on proton or dispatch (but I don't expect to get time
soon...). I don't buy the "mathematical" argument. Whether you use the
processors implementation of bit shifting or some other means of re-organizing
the bytes the fact remains you have two byte sequences that may or may not be
in different order. Shifting is a handy portable way of getting the CPU to do
it for you, but if it's not the fastest way I don't see any pragmatic reason to
prefer it. Shift is faster than simple copy/reverse, probably because the
compiler can optimize the shift half of the operation in-register, but the
other half is still an unaligned pointer iteration. Implementations like
byteswap use CPU-specific instructions to do the swap as an aligned load, a
single swap ope
ration in a register, and aligned save (where the CPU supports it) - that's
why they are faster. (Even if you add an unaligned-to-aligned memcpy, as my
benchmark does)
Not relevant to Qpid but if you ever deal with receiver-makes-right encodings
(like CORBA CDR or unicode) then shifting loses all its appeal. You no longer
have a fixed wire order and it is essential that you optimize the common case
where wire and host order match as a no-op. CDR goes even further and has
alignment rules so if you are clever and wire/host order match you can do
aligned copies or even (in principle) use pointers direct to network buffers in
some cases. That gets tricky though and I don't think most implementations go
that far. It's a pity AMQP didn't learn more from fast binary encodings like
CDR, we are doomed to be slow and fat compared to MQTT and the like.
- Alan
-----------------------------------------------------------
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
>
>
