Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20733 )
Change subject: IMPALA-11805: Use llvm ObjectCache for codegen caching ...................................................................... Patch Set 4: (12 comments) Thanks Daniel for reviewing. http://gerrit.cloudera.org:8080/#/c/20733/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20733/3//COMMIT_MSG@12 PS3, Line 12: which largely exceeds our memo > I think this should come before "which largely exceeds...", because "which Done http://gerrit.cloudera.org:8080/#/c/20733/3//COMMIT_MSG@17 PS3, Line 17: > Nit: space before '('. Done http://gerrit.cloudera.org:8080/#/c/20733/3//COMMIT_MSG@18 PS3, Line 18: pre-compiled : codegened > Do you mean the codegened functions? First I thought it referred to the fun Yeah, changed it to pre-compiled codegened functions. http://gerrit.cloudera.org:8080/#/c/20733/3//COMMIT_MSG@28 PS3, Line 28: > Could you also include the aggregate result over all queries? Do you mean run the time running all the tpch queries for once? Did a test, run only once for all the TPCH queries, the result of TPCH-Q1 for Execution Engine looks suspicious (maybe because it is the first of all to run), but all others look similar. Query Execution Engine Object Cache Q1 4 1.68 Q10 1.52 1.67 Q11 0.8 0.8 Q12 0.68 0.58 Q13 1.09 1.29 Q14 0.82 0.82 Q15 0.89 0.88 Q16 0.8 0.75 Q17 1.09 1.09 Q18 1.15 1.25 Q19 2.07 2.11 Q2 1.36 1.36 Q20 1.05 1.06 Q21 1.91 1.91 Q22 0.63 0.63 Q3 0.94 0.93 Q4 0.48 0.47 Q5 1.45 1.45 Q6 0.26 0.26 Q7 2.2 2.15 Q8 1.81 1.81 Q9 1.85 1.85 Total 28.85 26.8 http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache-test.cc File be/src/codegen/llvm-codegen-cache-test.cc: http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache-test.cc@99 PS3, Line 99: dule_id, bool > We could include the parameter names here and on the next line if 'codegen' Done http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache-test.cc@115 PS3, Line 115: > Maybe 'cached_engine' would be better, also on L117. Done http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache-test.cc@132 PS3, Line 132: > I'd prefer something like 'codegen_obj_cache_', it's easier to get what it Done http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache-test.cc@138 PS3, Line 138: > Could it be const? Done http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache-test.cc@142 PS3, Line 142: > Could use 'nullptr' instead. Done http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache-test.cc@225 PS3, Line 225: te a Llvm > 'module_id_echo' would be better, cf. 'module_id_double'. Done http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache.h File be/src/codegen/llvm-codegen-cache.h: http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache.h@231 PS3, Line 231: engine_cache_raw > Maybe 'engine_cache_raw[_ptr]' would be better. Done http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache.cc File be/src/codegen/llvm-codegen-cache.cc: http://gerrit.cloudera.org:8080/#/c/20733/3/be/src/codegen/llvm-codegen-cache.cc@154 PS3, Line 154: Slice key; > The assignment could be made in an IF-ELSE (or a ? : operator). Now we get/ Done -- To view, visit http://gerrit.cloudera.org:8080/20733 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3c1b46bb9018ed0320817141785a3bdc41fa677 Gerrit-Change-Number: 20733 Gerrit-PatchSet: 4 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Sat, 02 Dec 2023 13:15:54 +0000 Gerrit-HasComments: Yes
