Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8587 )
Change subject: schema: micro-optimize num_columns() ...................................................................... schema: micro-optimize num_columns() Looking at some profile earlier this week I noticed some 'imul' instructions showing up where I didn't expect them. I traced this back to calls to schema.num_columns() which I assumed was a simple load. Rather, it turns out that vector::size() does 'vector.end() - vector.start()', which has to divide by the size of the element in order to calculate the size. Since ColumnSchema has a somewhat odd size (72 bytes at the time of writing), the division-by-72 gets implemented by a multiplication of its inverse, rather than something cheaper like a bitshift. Simpler, though, is to just use a different mechanism for Schema::num_columns() which doesn't involve any calculation. Didn't benchmark this since I'm not sure which benchmark might highlight the change, but looking at generated assembly of CopyRow in release mode shows the difference. Before, the loop condition looked like: 0x0000000001a2f0f8 <+328>: add $0x1,%r15 0x0000000001a2f0fc <+332>: mov %r12,%rdi 0x0000000001a2f0ff <+335>: mov (%rdi),%rax 0x0000000001a2f102 <+338>: mov 0x8(%rax),%rcx 0x0000000001a2f106 <+342>: sub (%rax),%rcx 0x0000000001a2f109 <+345>: sar $0x5,%rcx 0x0000000001a2f10d <+349>: movabs $0xaaaaaaaaaaaaaaab,%rdx 0x0000000001a2f117 <+359>: imul %rdx,%rcx 0x0000000001a2f11b <+363>: movabs $0x100000000,%rdx 0x0000000001a2f125 <+373>: add %rdx,%r14 0x0000000001a2f128 <+376>: add $0xc,%r13 0x0000000001a2f12c <+380>: cmp %r15,%rcx 0x0000000001a2f12f <+383>: ja 0x1a2f000 <kudu::CopyRow<kudu::RowBlockRow, kudu::ContiguousRow, kudu::Arena>(kudu::RowBlockRow const&, kudu::ContiguousRow*, kudu::Arena*)+80> After: 0x0000000001a2ea78 <+328>: add $0x1,%r15 0x0000000001a2ea7c <+332>: mov %r12,%rdi 0x0000000001a2ea7f <+335>: mov (%rdi),%rax 0x0000000001a2ea82 <+338>: movabs $0x100000000,%rcx 0x0000000001a2ea8c <+348>: add %rcx,%r14 0x0000000001a2ea8f <+351>: add $0xc,%r13 0x0000000001a2ea93 <+355>: cmp %r15,0x80(%rax) 0x0000000001a2ea9a <+362>: ja 0x1a2e980 <kudu::CopyRow<kudu::RowBlockRow, kudu::ContiguousRow, kudu::Arena>(kudu::RowBlockRow const&, kudu::ContiguousRow*, kudu::Arena*)+80> Change-Id: I52d63e17d77c0b35685aa3cfba51e6905308592d Reviewed-on: http://gerrit.cloudera.org:8080/8587 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin <[email protected]> --- M src/kudu/common/schema.h 1 file changed, 10 insertions(+), 4 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/8587 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I52d63e17d77c0b35685aa3cfba51e6905308592d Gerrit-Change-Number: 8587 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]>
