impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
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:

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
File be/src/exec/

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.
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 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.
File be/src/util/bloom-filter.h:

PS1, Line 176: inline void IR_ALWAYS_INLINE

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf0fdedabd488550b6db90167a30c582949d608d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message