Dan Hecht has posted comments on this change. Change subject: IMPALA-3066: Lazy materialization of LLVM module bitcode. ......................................................................
Patch Set 3: (13 comments) http://gerrit.cloudera.org:8080/#/c/3220/3/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS3, Line 212: false why do we want to eagerly materialize in this case? Line 226: } it looks like there are only two callers of this, one with false and one true. so maybe it'd be simpler to just move this if-else code into the callers and have the caller pass in the tmp_module? PS3, Line 227: UNLIKELY nit: this seems unnecessary. i think we should keep UNLIKELY only on perf sensitive paths to keep things more readable. PS3, Line 299: isMaterializable what does it mean to not be materializable? Line 547: VLOG_QUERY << "Skipping materialization of " << fn->getName().str(); how verbose is this logging? Line 553: LOG(ERROR) << "Failed to materialize " << fn->getName().str() << ": " << err.message(); is it okay to continue on this case? when does this happen? we don't need to propagate an error status? Line 557: DCHECK(!fn->isMaterializable()); what does this mean? that it's already been materialized? PS3, Line 574: MaterializeFunction from the name it was surprising that it materializes all reachable functions, not just this function. How about MaterializeReachableFunctions() or something? http://gerrit.cloudera.org:8080/#/c/3220/3/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: PS3, Line 249: referenced only by global variables if referenced by global variables, how do we know they are not reachable? Line 328: llvm::Function* GetFunction(const string& symbol); document this. e.g. does it return a clone or not? also this has the side effect of materializing this and all reachable functions, right? that should be documented. how are these different? why do we need both? PS3, Line 434: so callers should call : /// MaterializeFunction() when should they call that? if they are suppose to call it, why wouldn't LlvmCodeGen just call it for them? actually, now that i've read the code, it's not really the caller that does it, but doesn't it happen on demand when the function is retrieved by GetFunction() public interface? So how about just saying the functions are not materialized, but will be materialized when referenced by GetFunction(). or whatever the case is. i.e. it doesn't seem like callers really have to do anything special, the materialization still happens "automatically", no? Line 440: /// file. The caller is responsible for cleaning up module. let's document that these routines are eagerly materialized and why. Line 503: void MaterializeFunction(llvm::Function* fn); what is the difference between these? seems they could use separate comments so it's clear what they each do and why they're both needed without having to read the .cc file. -- 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: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
