Jim Apple has posted comments on this change. Change subject: IMPALA-2809: Improve ByteSwap with builtin function or SSSE3 or AVX2. ......................................................................
Patch Set 32: (9 comments) http://gerrit.cloudera.org:8080/#/c/3081/32/be/src/benchmarks/bswap-benchmark.cc File be/src/benchmarks/bswap-benchmark.cc: PS32, Line 1: 2015 2016 Line 46: /// OldImpala 1.224e+06 1X Can you rebase and rerun? There is a new benchmarking output system PS32, Line 62: case 1: : *reinterpret_cast<int8_t*>(dst) = *reinterpret_cast<const int8_t*>(src); : return; : case 2: : *reinterpret_cast<int16_t*>(dst) = : BitUtil::ByteSwap(*reinterpret_cast<const int16_t*>(src)); : return; : case 4: : *reinterpret_cast<int32_t*>(dst) = : BitUtil::ByteSwap(*reinterpret_cast<const int32_t*>(src)); : return; : case 8: : *reinterpret_cast<int64_t*>(dst) = : BitUtil::ByteSwap(*reinterpret_cast<const int64_t*>(src)); : return; These all look like dead code to me, which means this doesn't actually call BitUtil::ByteSwap Line 90: static inline void Verification() { Please give this function a comment Line 92: uint8_t* SourceData = new uint8_t[DataLen]; Don't use new explicitly if you don't have to. Try std::vector PS32, Line 118: Scala "Scalar", not "Scala", here and elsewhere Line 145: data.inbuffer = new uint8_t[datalen]; Again, please avoid new. http://gerrit.cloudera.org:8080/#/c/3081/32/be/src/util/bit-util.cc File be/src/util/bit-util.cc: Line 153: uint128_t part1 = *reinterpret_cast<uint128_t*>(dst); why not use __m128i? http://gerrit.cloudera.org:8080/#/c/3081/32/be/src/util/bit-util.h File be/src/util/bit-util.h: Line 250: void ByteSwapScalarLoop(void* dst, const void* src, int len); Is this used outside of this header? If not, please put in only in the .cc file. The same goes for ByteSwapSIMD and uint128_t. -- To view, visit http://gerrit.cloudera.org:8080/3081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I392ed5a8d5683f30f161282c228c1aedd7b648c1 Gerrit-PatchSet: 32 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang <youwei.a.w...@intel.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Youwei Wang <youwei.a.w...@intel.com> Gerrit-HasComments: Yes
