Make unaligned unmarshaling opt-in in C++ #2536
Conversation
| # Extra -I because we include <MyByteSeq.h> in the generated code. | ||
| # ICE_UNALIGNED means enable unaligned decoding for integral types such as int32_t. It requires a little endian CPU. | ||
| # TODO: should be turn on ICE_UNALIGNED only on some platforms? | ||
| $(test)_cppflags := -I$(project) -DICE_UNALIGNED |
There was a problem hiding this comment.
We also need to define ICE_UNALIGNED on Windows. I'll do this later as I currently don't have access to a Windows computer.
| #ifdef ICE_UNALIGNED | ||
| // Verify inSeq is not aligned. | ||
| test(reinterpret_cast<size_t>(in.first) % 8 != 0); | ||
| test(*(in.first) == 3.14); |
There was a problem hiding this comment.
Not sure this works everywhere. It works on macos arm64.
There was a problem hiding this comment.
Ice for Python took advantage of this optimization for unmarshaling sequences
See
ice/python/modules/IcePy/Types.cpp
Line 2120 in df641f7
Maybe we should enable it for Python builds.
| @@ -591,7 +583,8 @@ namespace Ice | |||
| } | |||
| } | |||
|
|
|||
| #ifdef ICE_UNALIGNED // Optimization with unaligned reads | |||
| #if defined(ICE_UNALIGNED) || (defined(_WIN32) && defined(ICE_API_EXPORTS)) | |||
There was a problem hiding this comment.
On Windows, we need to build & export these functions all the time, otherwise they aren't available when we "opt in" to get them.
|
|
||
| using namespace std; | ||
| using namespace Ice; | ||
| using namespace Test; | ||
|
|
||
| void | ||
| TestIntfI::opShortArrayAsync( | ||
| pair<const int16_t*, const int16_t*> inSeq, |
There was a problem hiding this comment.
don't we need to add padding here too?
There was a problem hiding this comment.
No, the padding is a function of the number of previous bytes in the request message (14 for the header, N for the operation name etc.).
| @@ -591,7 +583,8 @@ namespace Ice | |||
| } | |||
| } | |||
|
|
|||
| #ifdef ICE_UNALIGNED // Optimization with unaligned reads | |||
| #if defined(ICE_UNALIGNED) || (defined(_WIN32) && defined(ICE_API_EXPORTS)) | |||
There was a problem hiding this comment.
What about renaming this macro to ICE_SUPPORT_UNALIGMENT_READ
There was a problem hiding this comment.
I prefer a short / easier to type name. And it's not about supporting unaligned reads, but about wanting to use them.
This PR makes the unaligned unmarshaling of some cpp:array sequences opt-in in C++.
It also improves the custom test.