impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5273: Replace StringCompare with glibc memcmp
Date Fri, 05 May 2017 17:08:02 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5273: Replace StringCompare with glibc memcmp
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6768/2/be/src/benchmarks/string-compare-benchmark.cc
File be/src/benchmarks/string-compare-benchmark.cc:

PS2, Line 202:  suite.AddBenchmark("Original", TestStringCompare<StringCompare1>, &data);
             :   suite.AddBenchmark("Simplified, broken", TestStringCompare<StringCompare2>,
&data);
             :   suite.AddBenchmark("Simplified, fixed, strncmp",
             :       TestStringCompare<StringCompare3<char, strncmp>>, &data);
             :   suite.AddBenchmark("Simplified, fixed, memcmp",
             :       TestStringCompare<StringCompare3<void, memcmp>>, &data);
> No SSE 4.2, Simplified, fixed, memcmp.
Sure. Or why do we even need 1,2,3? I haven't looked at this in enough detail to understand
what those are all about, but at this point in time, it seems to me the interesting comparisons
are:

1) StringCompare as implemented in Impala before your change.
2) strcncmp
3) memcmp.

If you think there is value in keeping the other StringCompare versions, I'm fine with that
too. But curious what it is.


-- 
To view, visit http://gerrit.cloudera.org:8080/6768
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4786a4a75fdaffedd6e17cf076b5368ba4b4e3e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message