Michael Ho has posted comments on this change. Change subject: IMPALA-4929: Safe concurrent access to IR function call graph ......................................................................
Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/6326/1/be/src/codegen/codegen-callgraph.cc File be/src/codegen/codegen-callgraph.cc: Line 27: using namespace strings; > You can use common/names.h instead of the string and unordered_* usings. Done Line 35: Status status = > Can combine this with the below line. Too long after converting to nullptr. PS1, Line 71: boost:: > I believe this creates two unordered_sets (the Lvalue and Rvalue), then cop Done Line 87: call_graph_[key].insert(fn_name); > I think we can use find(key)->insert here, since we don't really want the b The entry may not exist already. Note the entry creation above is for 'fn_name'. Auto-creating entries here is fine as we are constructing the map. We just don't want this to happen after initialization. http://gerrit.cloudera.org:8080/#/c/6326/1/be/src/codegen/codegen-callgraph.h File be/src/codegen/codegen-callgraph.h: Line 21: #include <boost/unordered_map.hpp> > Use std::unordered_set and _map when possible Good point but it appears that two closely related files llvm-codegen.cc and libcache.cc are already using boost::unordered_* so I'd avoid mixing both to avoid confusion or to further extend the scope of this change. We should clean up all these places in one shot instead. Line 29: class CodegenCallGraph { > One line class comment? Done PS1, Line 39: inline > I believe "inline" is implied for all methods defined in the class body: Done Line 43: if (LIKELY(iter != call_graph_.end())) { > Consider using ? : to make this a one-liner Done PS1, Line 46: NULL > nullptr? Done PS1, Line 51: inline > unnecessary inline Done Line 71: /// and return them in 'users'. > "append them to 'users'", to make it clear that it doesn't clear existing c Done http://gerrit.cloudera.org:8080/#/c/6326/1/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS1, Line 290: NULL > nit: we've mostly standardised on nullptr Done PS1, Line 594: (*callees) > nit: unnecessary parens Done -- To view, visit http://gerrit.cloudera.org:8080/6326 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
