You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/03/09 07:35:25 UTC
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Michael Ho has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6326
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
IMPALA-4929: Safe concurrent access to IR function call graph
Previously, LlvmCodeGen creates a map (fn_refs_map_) which
maps an IR function to its set of callees. That map was initialized
once at Impalad's start time and it is supposed to be read-only
afterwards.
However, the map was unintentionally modified by its user
LlvmCodeGen::MaterializeFunctionHelper() due to the use of
operator []. In particular, that operator may create a new
entry in the map if it doesn't exist already. This is possible
because the map initially only contains entries for functions
with at least one callee function. Using the operator [] with
functions with no callee function may modify the map. Since the
map is shared by all fragment instances, such unsafe concurrent
modification can cause missing or wrong callee functions to be
materialized, leading to failure to resolve symbols in LLVM.
This change fixes the problem above by:
1. Create an entry for all functions even if it has no callee.
In fact, LlvmCodeGen::LinkModule() assumes all functions
defined in main module will have entries in the map.
2. Introduce a new class CodegenCallGraph which encapsulates
the map described above. The map was established once at
initialization time and there is no interface to modify the
map afterwards, thus preventing any accidental modification
to the map at run time.
Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a
---
M be/src/codegen/CMakeLists.txt
A be/src/codegen/codegen-callgraph.cc
A be/src/codegen/codegen-callgraph.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
5 files changed, 191 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/6326/1
--
To view, visit http://gerrit.cloudera.org:8080/6326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6326/1/be/src/codegen/codegen-callgraph.cc
File be/src/codegen/codegen-callgraph.cc:
Line 87: call_graph_[key].insert(fn_name);
> The entry may not exist already. Note the entry creation above is for 'fn_n
Ah got it
http://gerrit.cloudera.org:8080/#/c/6326/2/be/src/codegen/codegen-callgraph.cc
File be/src/codegen/codegen-callgraph.cc:
PS2, Line 71: <std:
std:: not needed.
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>
> Good point but it appears that two closely related files llvm-codegen.cc an
I believe the std:: and boost:: versions have different perf characteristics because the c++ standard has some restrictions around how iterators behave. So they might not be a simple drop-in replacement.
E.g.
https://tinodidriksen.com/2010/04/cpp-set-performance/
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6326/2/be/src/codegen/codegen-callgraph.cc
File be/src/codegen/codegen-callgraph.cc:
PS2, Line 71: std::
> std:: not needed.
Done
http://gerrit.cloudera.org:8080/#/c/6326/2/be/src/codegen/codegen-callgraph.h
File be/src/codegen/codegen-callgraph.h:
Line 57: CallGraph call_graph_;
> Maybe this is a bit overkill, but moving this (and the Init() function) to
Thanks for the idea. The public interfaces of this class: GetCallees() and fns_referenced_by_gv() both have const attribute in their return type so it should get most of what we need for preventing mutation outside of the class. Of course, one can always do const_cast() but I guess we should catch them in code review.
The bool inited_ will catch re-initialization of the map as this should not occur.
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6326
to look at the new patch set (#3).
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
IMPALA-4929: Safe concurrent access to IR function call graph
Previously, LlvmCodeGen creates a map (fn_refs_map_) which
maps an IR function to its set of callees. That map was initialized
once at Impalad's start time and it is supposed to be read-only
afterwards.
However, the map was unintentionally modified by its user
LlvmCodeGen::MaterializeFunctionHelper() due to the use of
operator []. In particular, that operator may create a new
entry in the map if it doesn't exist already. This is possible
because the map initially only contains entries for functions
with at least one callee function. Using the operator [] with
functions with no callee function may modify the map. Since the
map is shared by all fragment instances, such unsafe concurrent
modification can cause missing or wrong callee functions to be
materialized, leading to failure to resolve symbols in LLVM.
This change fixes the problem above by:
1. Create an entry for all functions even if it has no callee.
In fact, LlvmCodeGen::LinkModule() assumes all functions
defined in main module will have entries in the map.
2. Introduce a new class CodegenCallGraph which encapsulates
the map described above. The map was established once at
initialization time and there is no interface to modify the
map afterwards, thus preventing any accidental modification
to the map at run time.
Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a
---
M be/src/codegen/CMakeLists.txt
A be/src/codegen/codegen-callgraph.cc
A be/src/codegen/codegen-callgraph.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
5 files changed, 188 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/6326/3
--
To view, visit http://gerrit.cloudera.org:8080/6326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
Patch Set 1:
(1 comment)
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
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
Patch Set 3:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/367/
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
Patch Set 1:
(12 comments)
Thanks for the cleanup - I think this makes it easier to understand the callgraph
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.
Line 35: Status status =
Can combine this with the below line.
PS1, Line 71: boost::
I believe this creates two unordered_sets (the Lvalue and Rvalue), then copies the rvalue to the lvalue then destroys the rvalue. I think if you use emplace(fn_name) that just initialises the set inside the map: http://en.cppreference.com/w/cpp/container/unordered_map/emplace
The boost:: package name is also not needed.
Line 87: call_graph_[key].insert(fn_name);
I think we can use find(key)->insert here, since we don't really want the behaviour of auto-creating entries.
http://gerrit.cloudera.org:8080/#/c/6326/1/be/src/codegen/codegen-callgraph.h
File be/src/codegen/codegen-callgraph.h:
Line 29: class CodegenCallGraph {
One line class comment?
PS1, Line 39: inline
I believe "inline" is implied for all methods defined in the class body:
https://isocpp.org/wiki/faq/inline-functions#inline-member-fns-more
Line 43: if (LIKELY(iter != call_graph_.end())) {
Consider using ? : to make this a one-liner
PS1, Line 46: NULL
nullptr?
PS1, Line 51: inline
unnecessary inline
Line 71: /// and return them in 'users'.
"append them to 'users'", to make it clear that it doesn't clear existing contents.
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
PS1, Line 594: (*callees)
nit: unnecessary parens
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2).
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
IMPALA-4929: Safe concurrent access to IR function call graph
Previously, LlvmCodeGen creates a map (fn_refs_map_) which
maps an IR function to its set of callees. That map was initialized
once at Impalad's start time and it is supposed to be read-only
afterwards.
However, the map was unintentionally modified by its user
LlvmCodeGen::MaterializeFunctionHelper() due to the use of
operator []. In particular, that operator may create a new
entry in the map if it doesn't exist already. This is possible
because the map initially only contains entries for functions
with at least one callee function. Using the operator [] with
functions with no callee function may modify the map. Since the
map is shared by all fragment instances, such unsafe concurrent
modification can cause missing or wrong callee functions to be
materialized, leading to failure to resolve symbols in LLVM.
This change fixes the problem above by:
1. Create an entry for all functions even if it has no callee.
In fact, LlvmCodeGen::LinkModule() assumes all functions
defined in main module will have entries in the map.
2. Introduce a new class CodegenCallGraph which encapsulates
the map described above. The map was established once at
initialization time and there is no interface to modify the
map afterwards, thus preventing any accidental modification
to the map at run time.
Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a
---
M be/src/codegen/CMakeLists.txt
A be/src/codegen/codegen-callgraph.cc
A be/src/codegen/codegen-callgraph.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
5 files changed, 188 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/6326/2
--
To view, visit http://gerrit.cloudera.org:8080/6326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
Patch Set 3: Verified+1
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
Patch Set 2: Code-Review+2
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
IMPALA-4929: Safe concurrent access to IR function call graph
Previously, LlvmCodeGen creates a map (fn_refs_map_) which
maps an IR function to its set of callees. That map was initialized
once at Impalad's start time and it is supposed to be read-only
afterwards.
However, the map was unintentionally modified by its user
LlvmCodeGen::MaterializeFunctionHelper() due to the use of
operator []. In particular, that operator may create a new
entry in the map if it doesn't exist already. This is possible
because the map initially only contains entries for functions
with at least one callee function. Using the operator [] with
functions with no callee function may modify the map. Since the
map is shared by all fragment instances, such unsafe concurrent
modification can cause missing or wrong callee functions to be
materialized, leading to failure to resolve symbols in LLVM.
This change fixes the problem above by:
1. Create an entry for all functions even if it has no callee.
In fact, LlvmCodeGen::LinkModule() assumes all functions
defined in main module will have entries in the map.
2. Introduce a new class CodegenCallGraph which encapsulates
the map described above. The map was established once at
initialization time and there is no interface to modify the
map afterwards, thus preventing any accidental modification
to the map at run time.
Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a
Reviewed-on: http://gerrit.cloudera.org:8080/6326
Reviewed-by: Michael Ho <kw...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/codegen/CMakeLists.txt
A be/src/codegen/codegen-callgraph.cc
A be/src/codegen/codegen-callgraph.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
5 files changed, 188 insertions(+), 81 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Michael Ho: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
Patch Set 3: Code-Review+2
Carry Tim's +2 forward.
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6326/2/be/src/codegen/codegen-callgraph.h
File be/src/codegen/codegen-callgraph.h:
Line 57: bool inited_;
Maybe this is a bit overkill, but moving this (and the Init() function) to a nested class that only allows access to the map through a const reference (returned by .get()) would make it impossible to mutate the map after initialization, and the compiler will catch you at it if you try.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes