Dan Hecht has posted comments on this change. Change subject: IMPALA-3115: Hoist some variables out of loops in cross-compiled code. ......................................................................
Patch Set 7: (12 comments) Thanks, that looks a lot simpler. most just minor style things now. One thing we should check though is whether llvm is able to allocate the row_ and row_batch_end_ pointers to registers. http://gerrit.cloudera.org:8080/#/c/2661/7/be/src/exec/partitioned-aggregation-node-ir.cc File be/src/exec/partitioned-aggregation-node-ir.cc: Line 172: out_row or just use out_batch_iterator.Get() here, but okay to ignore. http://gerrit.cloudera.org:8080/#/c/2661/7/be/src/exec/partitioned-hash-join-node-ir.cc File be/src/exec/partitioned-hash-join-node-ir.cc: Line 50: out_row or just use out_batch_iterator.Get() in place of this variable, but okay to ignore. it's slightly less error prone to do that since if someone were to add another Next() call they couldn't forget to update out_row also. Line 52: RowBatch::Iterator probe_batch_iterator(probe_batch_.get(), probe_batch_pos_); Maybe comment that this iterator corresponds to probe_batch_pos_ which is always one ahead of current_probe_row_? http://gerrit.cloudera.org:8080/#/c/2661/7/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 1024: out_row consider getting rid of this. might help register pressure assuming to allocates the iterator fields to registers (does it?) http://gerrit.cloudera.org:8080/#/c/2661/7/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: Line 149: Last Pointer to the row after the Batch::num_rows() row, for read iterators. Line 150: RowBatch* const parent_; we generally put the fields after the methods. also, add brief comments before each field and blank lines like (most) other class definitions. Line 153: /// An iterator for going through a row batch, starting at 'row_idx'. this should be the class comment. Line 157: num_tuples_per_row_ * parent->num_rows_ reverse this or previous line to make them consistent. Line 171: DCHECK DCHECK_LE Line 172: reinterpret_cast<TupleRow*>(row_); could be: Get(); Line 175: /// Useful for read iterators to determine its limit. Returns true once the iterator has traversed 'RowBatch::num_rows() - row_idx' rows. Useful for read iterators. Write iterators should use RowBatch::AtCapacity() instead. Line 387: /// Macro for iterating through '_row_batch', starting at '_start_row_idx'. document _row: '_row' is a TupleRow* for the current row, or something. -- To view, visit http://gerrit.cloudera.org:8080/2661 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7152b1fb094b3c3574d203e3774f4297f2225dc Gerrit-PatchSet: 7 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: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
