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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-HasComments: Yes
