impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-2809: Improve scalar ByteSwap().
Date Thu, 12 May 2016 22:27:25 GMT
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message