Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9078 )
Change subject: IMPALA-6422: Use ldexp() instead of powf() in HLL. ...................................................................... Patch Set 2: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/9078/1/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/9078/1/be/src/exprs/aggregate-functions-ir.cc@1489 PS1, Line 1489: // TODO: Consider improving this loop (e.g. replacing 'if' with arithmetic op). Remove TODO while we're here? Seems more misleading than helpful since it's suggesting optimising something that clearly wasn't a bottleneck. In any case both clang and gcc compiled to an adc instruction rather than a branch: https://godbolt.org/g/qckbSj http://gerrit.cloudera.org:8080/#/c/9078/1/be/src/exprs/aggregate-functions-ir.cc@1512 PS1, Line 1512: uint64_t estimate = HllFinalEstimate(src.ptr, src.len); I believe src.len is always HLL_LEN here. Might be worth passing the constant directly to see if the body of the function can be optimised any better, e.g. with unrolled loops. -- To view, visit http://gerrit.cloudera.org:8080/9078 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I517614d3f9cf1cf56b15a173c3cfe76e0f2e0382 Gerrit-Change-Number: 9078 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Fri, 19 Jan 2018 18:01:18 +0000 Gerrit-HasComments: Yes
