impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Date Mon, 24 Oct 2016 21:26:23 GMT
Jim Apple has posted comments on this change.

Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................


Patch Set 7:

(8 comments)

> (8 comments)
 > 
 > The commit message doesn't really tell the full story here. In
 > particular, you should say something about AlignmentNew and what it
 > is for.

Done

 > 
 > The rearrangement of class members to group by type seems like it
 > sacrifices readability for performance in some strange cases  - the
 > statestore is a good example, where there is only ever one
 > statestore and it is not (as far as we know) highly sensitive to
 > its in-memory layout. What criteria did you use to decide that a
 > class' members should be grouped-by-type?

I rearranged them all. Tim convinced me to roll some back.

http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/common/init.cc
File be/src/common/init.cc:

PS6, Line 100: [[noreturn]]
> can this go on the previous line? It might be easier to read that way (simi
While it can, clang-format "fixes" it to this right away.


http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

PS6, Line 173: 
             : 
> Why remove these comments?
Accident.


PS6, Line 176: 
> why remove this?
Put it back in the definition in the class.


http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/exprs/aggregate-functions.h
File be/src/exprs/aggregate-functions.h:

PS6, Line 182: ec
> can you replace that with HLL_PRECISION?
Done


http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:

PS6, Line 140: uniqu
> remove std:: in cc files
Removed here and elsewhere, except for std::move. I see that usually referred to with the
prefix by C++ people. Thoughts?


http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:

PS6, Line 82: CacheLineAligned
> why?
subscriber_heartbeat_threadpool_ is a ThreadPool<T>, and ThreadPool<T>::work_queue_
is a BlockingQueue<S>, and BlcokingQueue<S> is marked CACHE_ALIGNED, presumably
to avoid false sharing.

I made CacheLineAligned use the C++11 alignas specifier and deleted CACHE_ALIGNED.


PS6, Line 253: boost::mutex exit_flag_lock_;
> This is a good example of where logical grouping is broken by grouping-by-t
rolled back


http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/util/aligned-new.h
File be/src/util/aligned-new.h:

PS6, Line 27: Objects that should be allocate
> What does 'must be allocated' mean - for correctness or performance or both
either - performance for BlockingQueue and correctness for SIMD values when using aligned
loads or stores.


-- 
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message