Michael Ho has posted comments on this change. Change subject: IMPALA-3066: Lazy materialization of LLVM module bitcode. ......................................................................
Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/3220/1/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 76: DEFINE_bool(disable_lazy_materialization, false, > Is there any reason to disable it? It seems like it's strictly better so lo In case anything breaks, we can revert to eager materialization mode. Line 97: static StringRef llvm_module_ir; > It seems unnecessary to introduce more globals. These are local to this file so not too bad IMHO. They are initialized once on impala start-up. Line 565: if (called_fn != NULL) MaterializeFunctionHelper(called_fn); > This would blow up if it was called on a recursive function, right? THis mi Not really. We do check if the function has been materialized already above in line 543. LLVM will set the function to be not materializable once it's materialized. DCHECK is added for clarification. Line 582: MaterializeFunction(fn); > This could be skipped if we're not lazy-loading right? Or if you implemente Line 543 will catch it anyway so it's not that bad. Line 741: MaterializeFunction(fn); > This is redundant on some call paths, right? Line 543 is a quick check that helps us return early if the function is already materialized which is always the case when lazy materialization is disabled. Line 785: if (!FLAGS_disable_lazy_materialization) { > Consider factoring this out into its own function. FInalizeModule() is alre Good idea. Line 797: nullptr > NULL? LLVM uses nullptr. Don't think this is actually the same as NULL. Line 798: nullptr > NULL? See reply above. http://gerrit.cloudera.org:8080/#/c/3220/1/be/src/runtime/buffered-tuple-stream.inline.h File be/src/runtime/buffered-tuple-stream.inline.h: Line 58 > This change seems reasonable, but is it related to lazy materialization? Not really. Just happened to see this opportunity when debugging some stuff. I can factor this out as a separate change. -- To view, visit http://gerrit.cloudera.org:8080/3220 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ed7862fc5e86005ecea83fa2ceb489e737d66b2 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
