Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/19181 )
Change subject: IMPALA-11470: Add Cache For Codegen Functions ...................................................................... Patch Set 8: (12 comments) Looks very good and the performance numbers are nice. Will find some time to finish review the rest of the code. http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen-cache.h File be/src/codegen/llvm-codegen-cache.h: http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen-cache.h@69 PS8, Line 69: HashCodeHash It seems this struct is used only for the operator() defined below. Can the operator() be defined in HashCode struct as getH1()? http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen-cache.h@70 PS8, Line 70: size_t Better to match the type for HashState.h1 (uint64_t). http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen-cache.h@76 PS8, Line 76: struct HashCodeEqual { : bool operator()(const HashCode& lhs, const HashCode& rhs) const { : return lhs.hash_code.h1 == rhs.hash_code.h1 && lhs.hash_code.h2 == rhs.hash_code.h2; : } : }; same as the above comment for struct HashCodeHash. http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen-cache.cc File be/src/codegen/llvm-codegen-cache.cc: http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen-cache.cc@162 PS8, Line 162: override the existing entry It is possible to use a strategy to retain a better entry than just taking the current one as the better one. An entry is better if it is more valuable in general. Also, since we allow 128Mb of memory for the cache, the LRU may be a good strategy to keep most valuable entries in the cache. http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen-cache.cc@171 PS8, Line 171: codegen_cache_entries_in_use_->Increment(1); : codegen_cache_entries_in_use_bytes_->Increment(mem_charge); This math is not correct if the old entry is replaced by a new one in hash collision case. http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen-cache.cc@183 PS8, Line 183: to_insert_key_it nit keys_to_insert_it http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen-cache.cc@185 PS8, Line 185: avoid the simultaneous insertion Can you please explain this logic maybe with some examples? http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen-cache.cc@188 PS8, Line 188: to_insert_keys_ nit. keys_to_insert http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen.h@194 PS8, Line 194: suc nit. This may not sound like a good name. http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen.h@715 PS8, Line 715: avoid nit. reduce http://gerrit.cloudera.org:8080/#/c/19181/8/be/src/codegen/llvm-codegen.h@766 PS8, Line 766: /// the codegen cache. May need to briefly describe how such a name list is obtained, in particular the order of these function names in the string. http://gerrit.cloudera.org:8080/#/c/19181/8/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/19181/8/common/thrift/ImpalaService.thrift@747 PS8, Line 747: // insert a full key for the cache and allow more statistics, otherwise, a hashcode of mere 64 bytes -- 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: 8 Gerrit-Owner: Yida Wu <wydbaggio...@gmail.com> Gerrit-Reviewer: Abhishek Rawat <ara...@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: Thu, 17 Nov 2022 19:27:56 +0000 Gerrit-HasComments: Yes
