[c++] Use memcpy to type pun; fix alias violations #375
Conversation
cpp/inc/bond/stream/input_buffer.h
Outdated
| value = *reinterpret_cast<const T*>(_blob.content() + _pointer); | ||
|
|
||
| const void* const src = _blob.content() + _pointer; | ||
| std::memcpy(&value, src, sizeof(T)); |
There was a problem hiding this comment.
TODO: see whether on x86 there's any performance regression due to this change in reading. If there is, add a guard and do the unaligned access like as before, like in output_buffer.
There was a problem hiding this comment.
I did some testing, and the reinterpret_cast code path is faster for amd64 MSVC, so I've added it back in the most recent push.
| @@ -19,18 +21,19 @@ struct VariableUnsignedUnchecked | |||
| { | |||
| BOOST_STATIC_ASSERT(N < 10); | |||
|
|
|||
| static uint32_t Write(uint8_t* p, T value) | |||
| static uint32_t Write(char* p, T value) | |||
There was a problem hiding this comment.
There was no measurable performance implication due to these changes when I tested.
| // which results in call to memcpy() and additional memory access. | ||
| // The direct copy of the value (which is likely on register already) | ||
| // is faster. | ||
| // x86/x64 performance tweak: for small types don't go into generic |
There was a problem hiding this comment.
I re-validated that this tweak still results in better performance--at least with MSVC.
| @@ -298,7 +314,7 @@ class OutputMemoryStream | |||
| { | |||
| if (sizeof(T) * 8 / 7 + _rangeSize + _rangeOffset < _bufferSize) | |||
| { | |||
| uint8_t* ptr = reinterpret_cast<uint8_t*>(_rangePtr + _rangeSize); | |||
| char* ptr = _rangePtr + _rangeSize; | |||
There was a problem hiding this comment.
The pointer can be const, but the thing it points to cannot. Not worth the potential confusion. Leaving as-is.
|
@glenfe, if you have time, I would appreciate if you could take a look at these changes. You're knowledge of the C++ standards is far better than mine. Thanks in advance. |
|
Looks fine to me. When reviewing the changes (but unrelated to your change) I noticed some things with the use of allocators: I'll create a proper Issue about it tomorrow. |
We now use memcpy to pun from one type to another, avoiding aliasing violations as well as alignment errors. x86-like chips never failed due to the alignment errors, but others, like some ARM chips, did. This should also fix emscripten cross-compilation. Fixes microsoft#305
chwarr
force-pushed
the
chwarr/cpp-type-puns
branch
from
1ba29cd to
4be8d35
Compare
|
Official AppVeyor builds are completely backed up due to some work @tstein is doing to improve the coverage we have on Travis for versions of Boost. (He's going to remove appveyor.yml in a subsequent change so that this doesn't keep happening.) This change passed on my AppVeyor config, with the usual timeout in the VC14 C++ build during tests. I ran all of these tests on my local machine. Going to merge. |
We now use memcpy to pun from one type to another, avoiding aliasing violations as well as alignment errors. x86-like chips never failed due to the alignment errors, but others, like some ARM chips, did. This should also fix emscripten cross-compilation. Fixes microsoft#305 Closes microsoft#375
We now use
memcpyto pun from one type to another, avoiding aliasingviolations as well as alignment errors. x86-like chips never failed due
to the alignment errors, but others, like some ARM chips, did. This
should also fix emscripten cross-compilation.
Fixes #305