Dan Hecht has posted comments on this change. Change subject: IMPALA-2809: Improve scalar ByteSwap(). ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/3033/3/be/src/util/bit-util.h File be/src/util/bit-util.h: Line 158: static_cast<uint32_t> I don't understand why we have casts here. AFAIK, bswap32 should return unsigned, so I'd expect the casts in the signed case (though the implicit conversion is okay too if the compiler doesn't warn. Line 164: return static_cast<uint16_t>(ByteSwap(static_cast<int16_t>(value))); if 16-bit byteswap is a common case, it might be worth seeing if we get better code doing: (value >> 8) | (value & 0xff) << 8 i.e. using logical shift rather than arithmetic & mask (though the compiler may already be optimizing the signed ByteSwap() to do this for us. http://gerrit.cloudera.org:8080/#/c/3033/3/be/src/util/bit-util.inline.h File be/src/util/bit-util.inline.h: Line 24: uint8_t why unsigned here but signed everywhere else? Line 28: int8_t why signed rather than unsigned? for byte manipulation, unsigned makes more sense IMO. Line 35: *reinterpret_cast<int16_t*>(dst + 1) = these loads/stores will be unaligned. on modern x86 that should be okay from a perf perspective but might not be true on other architectures. but that can be dealt with later as needed. Line 118: return; are all these cases exercised pretty well by impala? if not a simple unit test might be worth while -- To view, visit http://gerrit.cloudera.org:8080/3033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4f462e6bdb022db46b48889a6a7426120a80d9b4 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
