Hello Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8587
to review the following change.
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
---
M src/kudu/common/schema.h
1 file changed, 10 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/8587/1
--
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: newchange
Gerrit-Change-Id: I52d63e17d77c0b35685aa3cfba51e6905308592d
Gerrit-Change-Number: 8587
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
