impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3066: Lazy materialization of LLVM module bitcode.
Date Wed, 25 May 2016 22:47:06 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3066: Lazy materialization of LLVM module bitcode.
......................................................................


Patch Set 1:

(4 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,
> In case anything breaks, we can revert to eager materialization mode.
Maybe we should chat a bit offline. If there are some gaps in test coverage that mean we're
not confident in the lazy materialization, it would probably help to address them. My concern
is a) adding more options/complexity b) we won't be testing the "safe" option, so bugs could
creep in there.


Line 97: static StringRef llvm_module_ir;
> These are local to this file so not too bad IMHO. They are initialized once
It's still some additional effort to reason about when they are initialised, etc, and I don't
see any benefit.


Line 797: nullptr
> LLVM uses nullptr. Don't think this is actually the same as NULL.
There are some obscure type differences but I think NULL should work ok (it's what we use
everywhere).


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
> Not really. Just happened to see this opportunity when debugging some stuff
I'm ok with this, maybe just mention in commit message?


-- 
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 <kwho@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message