impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4231: fix codegen time regression
Date Thu, 06 Oct 2016 17:23:05 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4231: fix codegen time regression
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4623/1//COMMIT_MSG
Commit Message:

PS1, Line 26: y, which is fixed by this
            : patch.
> Does it recover all the performance ?
Locally it looked like it got back all of the codegen time plus some more. I've had trouble
getting a full cluster run completed, but it looks like it got TPC-H Q2 back to the 4.5 second
range.


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 thought the destructor is inlined so it only calls FreeMessage() which is
The codegen time was higher. Otherwise the runtime didn't seem to be noticeably higher. Looking
at the IR the Statuses added 5-10 additional basic blocks to the Process*Batch functions that
were already pretty huge. Basically every local Status variable or return of a Status variable
was adding calls to the Copy/FreeMsg function that weren't optimised out.

I posted the IR diff as patchsets 1 and 2 here.


http://gerrit.cloudera.org:8080/#/c/4623/1/be/src/runtime/raw-value.inline.h
File be/src/runtime/raw-value.inline.h:

PS1, Line 215: 
> this may need to be inlined again eventually for loop unrolling to work.
I agree, I think we should move it to the -ir.cc file once we're able to propagate constants
into it. It doesn't make sense to burn cycles recompiling the whole interpreted function all
the time. It will be cheaper to optimise the specialised version without the big switch.


http://gerrit.cloudera.org:8080/#/c/4623/1/be/src/util/bloom-filter.h
File be/src/util/bloom-filter.h:

PS1, Line 176: inline void IR_ALWAYS_INLINE
> void ALWAYS_INLINE
Done


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message