Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19181 )
Change subject: IMPALA-11470: Add Cache For Codegen Functions ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/19181/9/be/src/codegen/llvm-codegen-cache.h File be/src/codegen/llvm-codegen-cache.h: http://gerrit.cloudera.org:8080/#/c/19181/9/be/src/codegen/llvm-codegen-cache.h@69 PS9, Line 69: /// Used as the hash function in unordered_set for struct HashCode. This was a little confusing, a longer explanation would be helpful. We're using this as the hash key, but doing comparison based on the full hash. Is this a lookup speed optimization? http://gerrit.cloudera.org:8080/#/c/19181/9/be/src/codegen/llvm-codegen-cache.h@80 PS9, Line 80: return lhs.hash_code.h1 == rhs.hash_code.h1 && lhs.hash_code.h2 == rhs.hash_code.h2; This could be implemented in terms of lhs == rhs since you implemented operator== there. http://gerrit.cloudera.org:8080/#/c/19181/5/be/src/codegen/llvm-codegen-cache.h File be/src/codegen/llvm-codegen-cache.h: http://gerrit.cloudera.org:8080/#/c/19181/5/be/src/codegen/llvm-codegen-cache.h@70 PS5, Line 70: struct HashCodeHash { > Yes, set the first sizeof(HashCode) bytes of the key as the hash code. Done http://gerrit.cloudera.org:8080/#/c/19181/5/be/src/codegen/llvm-codegen-cache.h@81 PS5, Line 81: } > Done Done http://gerrit.cloudera.org:8080/#/c/19181/5/be/src/codegen/llvm-codegen-cache.h@92 PS5, Line 92: > I think the main purpose of setting a Constructor is to be able to collect This feels like pre-mature design, but I don't disagree strongly. http://gerrit.cloudera.org:8080/#/c/19181/5/be/src/codegen/llvm-codegen-cache.h@195 PS5, Line 195: > Done Done http://gerrit.cloudera.org:8080/#/c/19181/5/be/src/codegen/llvm-codegen-cache.cc File be/src/codegen/llvm-codegen-cache.cc: http://gerrit.cloudera.org:8080/#/c/19181/5/be/src/codegen/llvm-codegen-cache.cc@66 PS5, Line 66: string result; > Done I don't think this was actually addressed. What I was suggesting is to hash `result.c_str()+8, length-8` (or now +16/-16 because MurmurHash3_x64_128 is 128 bits). http://gerrit.cloudera.org:8080/#/c/19181/9/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/19181/9/be/src/codegen/llvm-codegen.cc@1138 PS9, Line 1138: LOG(WARNING) I wish we could guarantee no incorrect use, but this plus the increased entropy with http://gerrit.cloudera.org:8080/#/c/19181/9/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/19181/9/common/thrift/ImpalaService.thrift@748 PS9, Line 748: mere Phrasing here confused my, I think 'mere' is superfluous. -- To view, visit http://gerrit.cloudera.org:8080/19181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If42c78a7f51fd582e5fe331fead494dadf544eb1 Gerrit-Change-Number: 19181 Gerrit-PatchSet: 9 Gerrit-Owner: Yida Wu <wydbaggio...@gmail.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qfc...@hotmail.com> Gerrit-Reviewer: Yida Wu <wydbaggio...@gmail.com> Gerrit-Comment-Date: Tue, 22 Nov 2022 19:09:42 +0000 Gerrit-HasComments: Yes
