You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2015/04/14 23:55:33 UTC
Review Request 33194: QPID-6470: Fix float conversion problems.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33194/
-----------------------------------------------------------
Review request for qpid and Andrew Stitcher.
Repository: qpid
Description
-------
QPID-6470: Fix float conversion problems.
Diffs
-----
trunk/qpid/cpp/src/qpid/framing/Endian.h 1673017
trunk/qpid/cpp/src/qpid/framing/Endian.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
-------
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.
Thanks,
Alan Conway
Re: Review Request 33194: QPID-6470: Fix float conversion problems.
Posted by Alan Conway <ac...@redhat.com>.
> On April 15, 2015, 10:15 p.m., Andrew Stitcher wrote:
> > I much prefer this version - good work.
Me too, good reviewing :)
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33194/#review80272
-----------------------------------------------------------
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
>
>
Re: Review Request 33194: QPID-6470: Fix float conversion problems.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33194/#review80272
-----------------------------------------------------------
Ship it!
I much prefer this version - good work.
- Andrew Stitcher
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
>
>
Re: Review Request 33194: QPID-6470: Fix float conversion problems.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
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.
Changes
-------
Fix points raised by Andrew.
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 (updated)
-----
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
Re: Review Request 33194: QPID-6470: Fix float conversion problems.
Posted by Andrew Stitcher <as...@apache.org>.
> 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
>
>
Re: Review Request 33194: QPID-6470: Fix float conversion problems.
Posted by Alan Conway <ac...@redhat.com>.
> 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
>
>
Re: Review Request 33194: QPID-6470: Fix float conversion problems.
Posted by Alan Conway <ac...@redhat.com>.
> 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.
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.
> On April 15, 2015, 3:04 a.m., Andrew Stitcher wrote:
> > trunk/qpid/cpp/src/tests/FieldValue.cpp, line 93
> > <https://reviews.apache.org/r/33194/diff/1/?file=927608#file927608line93>
> >
> > Why shouldn't floats convert to ints? (unless they outside the representable range)
They should. Originally they were "converting" by bitwise copy which was bad, but I can easily convert them properly now.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33194/#review80147
-----------------------------------------------------------
On April 14, 2015, 9:57 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33194/
> -----------------------------------------------------------
>
> (Updated April 14, 2015, 9:57 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/qpid/framing/Endian.h 1673017
> trunk/qpid/cpp/src/qpid/framing/Endian.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
>
>
Re: Review Request 33194: QPID-6470: Fix float conversion problems.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33194/#review80147
-----------------------------------------------------------
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.
trunk/qpid/cpp/src/tests/FieldValue.cpp
<https://reviews.apache.org/r/33194/#comment129917>
Why shouldn't floats convert to ints? (unless they outside the representable range)
- Andrew Stitcher
On April 14, 2015, 9:57 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33194/
> -----------------------------------------------------------
>
> (Updated April 14, 2015, 9:57 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/qpid/framing/Endian.h 1673017
> trunk/qpid/cpp/src/qpid/framing/Endian.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
>
>
Re: Review Request 33194: QPID-6470: Fix float conversion problems.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33194/
-----------------------------------------------------------
(Updated April 14, 2015, 9:57 p.m.)
Review request for qpid and Andrew Stitcher.
Repository: qpid
Description (updated)
-------
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/qpid/framing/Endian.h 1673017
trunk/qpid/cpp/src/qpid/framing/Endian.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 (updated)
-------
Added unit tests for float conversions.
Thanks,
Alan Conway