Tim Armstrong has posted comments on this change. Change subject: IMPALA-4231: fix codegen time regression ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/4623/2/be/src/exec/partitioned-aggregation-node.h File be/src/exec/partitioned-aggregation-node.h: PS2, Line 479: noexcept > I wonder if the need for noexcept stems from the use of vector in these fun The invoke is inserted when there's a) a call to a function without the noexcept attribute that isn't in the module (since as far as the compiler knows it will throw an exception) and b) a local variable with a destructor. It that case it has to insert code to run the destructor in the case that it throws an exception. Condition a) was always true, but the additional local status variables made condition b) true for more function calls. http://gerrit.cloudera.org:8080/#/c/4623/1/be/src/exec/partitioned-hash-join-builder-ir.cc File be/src/exec/partitioned-hash-join-builder-ir.cc: PS1, Line 32: inline bool PhjBuilder::AppendRow( > I see. So, it appears that LLVM wasn't able to consolidate the multiple cal No it wasn't (the code is messy so it's hard to reason about exactly what happened). I wasn't able to make the regression go away without reverting the status changes. I would have saved myself a couple of hours of time if I'd been able to do that. It's also unclear whether the problem is the # of additional calls or the number of additional edges in the control-flow graph, since both could contribute to compile time. I tried various permutations of the std::move idea and they were a little helpful but generally still left lots of redundant Free/Copy calls in the code. The other changes were helpful and seemed like reasonable improvements so I left them in even if they weren't strictly necessary. http://gerrit.cloudera.org:8080/#/c/4623/2/be/src/runtime/buffered-tuple-stream.h File be/src/runtime/buffered-tuple-stream.h: PS2, Line 244: appending succee > the append succeeded Done PS2, Line 245: or returns; > ? Done Line 245: /// and sets 'status' to OK if appending failed but can be retried or returns; or > to OK; If the append failed Done PS2, Line 247: AddRow > AddRow() Done http://gerrit.cloudera.org:8080/#/c/4623/2/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: PS2, Line 176: inline > Do you still need inline here ? I was getting a warning about the function not being inlinable otherwise. -- To view, visit http://gerrit.cloudera.org:8080/4623 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idf0fdedabd488550b6db90167a30c582949d608d Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
