You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/03/28 23:01:28 UTC

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows 

Todd Lipcon has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9848


Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................

KUDU-2378. Fix unaligned loads of int128 from rows

Our row format doesn't ensure any alignment of data. This is usually fine on
x86 where unaligned loads of ints don't need any special instruction and
carry a relatively low performance penalty. However, loads of int128_t may
generate a 'movdqa' (aligned load) instruction which crashes if the
operand is not 16-byte aligned.

This fixes a bunch of spots where we are dereferencing a reinterpret_casted
int and replaces them with a new UnalignedLoad<> templated function. This
function delegates to the UNALIGNED_LOAD macros for <=64bit types, which
themselves are equivalent to *reinterpret_cast<T*> on x86. For int128_t,
and for floats/doubles, we delegate to memcpy. The compiler should already
treat memcpy as an intrinsic and this will end up being a single instruction[1]

[1] https://godbolt.org/g/iT8coU

Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
---
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/client/client-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/row.h
M src/kudu/common/types.h
M src/kudu/common/wire_protocol-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/gutil/port.h
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/tablet/cfile_set-test.cc
M src/kudu/tablet/concurrent_btree.h
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/util/group_varint-inl.h
M src/kudu/util/group_varint-test.cc
M src/kudu/util/inline_slice.h
21 files changed, 177 insertions(+), 110 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9848/1
-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/cfile/plain_block.h
File src/kudu/cfile/plain_block.h:

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/cfile/plain_block.h@27
PS6, Line 27: #include "kudu/util/coding.h"
> I think it says coding-inl.h should come before coding.h  That sounds reaso
Oh I see. I expected the comment to be based on the changed line.



-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Apr 2018 02:37:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/common/partial_row.cc@191
PS6, Line 191:       // TODO(todd) Is reinterpret_cast unsafe here?
> Is this still an open item?
yea I think it should probably be doing an UnalignedLoad since technically it's illegal to do this typecast. It works fine on x86 since the underlying fields of slice are 64-bit and 64-bit unaligned loads are fine without any special assembly, but if we tried to run this on some other system it could crash. In fact with UBSAN "alignment" enabled, it does fail here. I have a separate WIP patch to fix all the 'alignment' issues with UBSAN but not really that critical given we only target x86


http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/common/types.h
File src/kudu/common/types.h:

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/common/types.h@110
PS6, Line 110:   } else if (lhs_int > rhs_int) {
> warning: do not use 'else' after 'return' [readability-else-after-return]
this is addressed by some other patch I have in flight


http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/gutil/port.h
File src/kudu/gutil/port.h:

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/gutil/port.h@1222
PS6, Line 1222:   memcpy(dst, &src, sizeof(T));
> Why additional memcpy() is necessary when USE_REINTERPRET is set?  If it's 
good catch!



-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Apr 2018 03:09:07 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/gutil/port.h
File src/kudu/gutil/port.h:

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/gutil/port.h@1222
PS6, Line 1222:   memcpy(dst, &src, sizeof(T));
Why additional memcpy() is necessary when USE_REINTERPRET is set?  If it's not a typo, please add a comment explaining that.



-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Apr 2018 01:58:23 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9848/1/src/kudu/gutil/port.h
File src/kudu/gutil/port.h:

http://gerrit.cloudera.org:8080/#/c/9848/1/src/kudu/gutil/port.h@1170
PS1, Line 1170: #if defined(__i386) || defined(ARCH_ATHLON) || defined(__x86_64__) || defined(_ARCH_PPC)
Do we actually care about ARCH_ATHLON and _ARCH_PPC? Or was this sourced from somewhere else?


http://gerrit.cloudera.org:8080/#/c/9848/1/src/kudu/gutil/port.h@1187
PS1, Line 1187:          typename port_internal::enable_if_numeric<T>::type* = nullptr,
What does this template parameter do? I don't see 'type' referenced in the function at all.



-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 28 Mar 2018 23:34:41 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 7: Code-Review+1


-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Apr 2018 17:56:44 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................

KUDU-2378. Fix unaligned loads of int128 from rows

Our row format doesn't ensure any alignment of data. This is usually fine on
x86 where unaligned loads of ints don't need any special instruction and
carry a relatively low performance penalty. However, loads of int128_t may
generate a 'movdqa' (aligned load) instruction which crashes if the
operand is not 16-byte aligned.

This fixes a bunch of spots where we are dereferencing a reinterpret_casted
int and replaces them with new UnalignedLoad<> and UnalignedStore<>
template functions. These use simple reinterpret_cast dereferences for
<=64bit types. For int128_t we use memcpy. The compiler should already
treat memcpy as an intrinsic and this will end up being a single
instruction.

Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Reviewed-on: http://gerrit.cloudera.org:8080/9848
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Grant Henke <gr...@apache.org>
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/common/partial_row.cc
M src/kudu/common/types.h
M src/kudu/gutil/port.h
M src/kudu/integration-tests/all_types-itest.cc
8 files changed, 93 insertions(+), 45 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, but someone else must approve
  Grant Henke: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 8
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/cfile/plain_block.h
File src/kudu/cfile/plain_block.h:

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/cfile/plain_block.h@27
PS6, Line 27: #include "kudu/util/coding.h"
> warning: #includes are not sorted properly [llvm-include-order]
looks right to me.


http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/common/partial_row.cc@191
PS6, Line 191:       // TODO(todd) Is reinterpret_cast unsafe here?
Is this still an open item?


http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/common/partial_row.cc@196
PS6, Line 196:       // TODO(todd) Is reinterpret_cast unsafe here?
Is this still an open item?



-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Apr 2018 00:47:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/9848

to look at the new patch set (#2).

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................

KUDU-2378. Fix unaligned loads of int128 from rows

Our row format doesn't ensure any alignment of data. This is usually fine on
x86 where unaligned loads of ints don't need any special instruction and
carry a relatively low performance penalty. However, loads of int128_t may
generate a 'movdqa' (aligned load) instruction which crashes if the
operand is not 16-byte aligned.

This fixes a bunch of spots where we are dereferencing a reinterpret_casted
int and replaces them with a new UnalignedLoad<> templated function. This
function delegates to the UNALIGNED_LOAD macros for <=64bit types, which
themselves are equivalent to *reinterpret_cast<T*> on x86. For int128_t,
and for floats/doubles, we delegate to memcpy. The compiler should already
treat memcpy as an intrinsic and this will end up being a single instruction[1]

[1] https://godbolt.org/g/iT8coU

Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
---
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/client/client-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/row.h
M src/kudu/common/types.h
M src/kudu/common/wire_protocol-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/gutil/port.h
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/tablet/cfile_set-test.cc
M src/kudu/tablet/concurrent_btree.h
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/util/group_varint-inl.h
M src/kudu/util/group_varint-test.cc
M src/kudu/util/inline_slice.h
21 files changed, 180 insertions(+), 110 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9848/2
-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 5:

here's a simpler one where I only changed the cases with int128 and with templatized types (which might be int128)


-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Apr 2018 00:16:56 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 3:

Let me know if you want me to post a patch which _only_ addresses the int128 issues (i.e. a more minimal patch). Perhaps this is better as it would be more backportable and I can address the wider issue separately at a lower priority?


-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 03 Apr 2018 00:01:17 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 7: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Apr 2018 19:23:45 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/cfile/plain_block.h
File src/kudu/cfile/plain_block.h:

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/cfile/plain_block.h@27
PS6, Line 27: #include "kudu/util/coding-inl.h"
> Oh I see. I expected the comment to be based on the changed line.
Done


http://gerrit.cloudera.org:8080/#/c/9848/1/src/kudu/gutil/port.h
File src/kudu/gutil/port.h:

http://gerrit.cloudera.org:8080/#/c/9848/1/src/kudu/gutil/port.h@1170
PS1, Line 1170: #if defined(__i386) || defined(ARCH_ATHLON) || defined(__x86_64__) || defined(_ARCH_PPC)
> Do we actually care about ARCH_ATHLON and _ARCH_PPC? Or was this sourced fr
I switched to using the #define that is already defined up above instead of repeating this set


http://gerrit.cloudera.org:8080/#/c/9848/1/src/kudu/gutil/port.h@1187
PS1, Line 1187:          typename port_internal::enable_if_numeric<T>::type* = nullptr,
> What does this template parameter do? I don't see 'type' referenced in the 
this is magic enable_if. See http://en.cppreference.com/w/cpp/types/enable_if example #4



-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Apr 2018 05:29:45 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/9848

to look at the new patch set (#7).

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................

KUDU-2378. Fix unaligned loads of int128 from rows

Our row format doesn't ensure any alignment of data. This is usually fine on
x86 where unaligned loads of ints don't need any special instruction and
carry a relatively low performance penalty. However, loads of int128_t may
generate a 'movdqa' (aligned load) instruction which crashes if the
operand is not 16-byte aligned.

This fixes a bunch of spots where we are dereferencing a reinterpret_casted
int and replaces them with new UnalignedLoad<> and UnalignedStore<>
template functions. These use simple reinterpret_cast dereferences for
<=64bit types. For int128_t we use memcpy. The compiler should already
treat memcpy as an intrinsic and this will end up being a single
instruction.

Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/common/partial_row.cc
M src/kudu/common/types.h
M src/kudu/gutil/port.h
M src/kudu/integration-tests/all_types-itest.cc
8 files changed, 93 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9848/7
-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/cfile/plain_block.h
File src/kudu/cfile/plain_block.h:

http://gerrit.cloudera.org:8080/#/c/9848/6/src/kudu/cfile/plain_block.h@27
PS6, Line 27: #include "kudu/util/coding.h"
> looks right to me.
I think it says coding-inl.h should come before coding.h  That sounds reasonable according 'man ascii'.



-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Apr 2018 02:36:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/9848

to look at the new patch set (#6).

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................

KUDU-2378. Fix unaligned loads of int128 from rows

Our row format doesn't ensure any alignment of data. This is usually fine on
x86 where unaligned loads of ints don't need any special instruction and
carry a relatively low performance penalty. However, loads of int128_t may
generate a 'movdqa' (aligned load) instruction which crashes if the
operand is not 16-byte aligned.

This fixes a bunch of spots where we are dereferencing a reinterpret_casted
int and replaces them with new UnalignedLoad<> and UnalignedStore<>
template functions. These use simple reinterpret_cast dereferences for
<=64bit types. For int128_t we use memcpy. The compiler should already
treat memcpy as an intrinsic and this will end up being a single
instruction.

Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/common/partial_row.cc
M src/kudu/common/types.h
M src/kudu/gutil/port.h
M src/kudu/integration-tests/all_types-itest.cc
8 files changed, 92 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9848/6
-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/9848

to look at the new patch set (#5).

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................

KUDU-2378. Fix unaligned loads of int128 from rows

Our row format doesn't ensure any alignment of data. This is usually fine on
x86 where unaligned loads of ints don't need any special instruction and
carry a relatively low performance penalty. However, loads of int128_t may
generate a 'movdqa' (aligned load) instruction which crashes if the
operand is not 16-byte aligned.

This fixes a bunch of spots where we are dereferencing a reinterpret_casted
int and replaces them with new UnalignedLoad<> and UnalignedStore<>
template functions. These use simple reinterpret_cast dereferences for
<=64bit types. For int128_t we use memcpy. The compiler should already
treat memcpy as an intrinsic and this will end up being a single
instruction.

Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/common/partial_row.cc
M src/kudu/common/types.h
M src/kudu/gutil/port.h
M src/kudu/integration-tests/all_types-itest.cc
8 files changed, 93 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9848/5
-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2378. Fix unaligned loads of int128 from rows

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9848 )

Change subject: KUDU-2378. Fix unaligned loads of int128 from rows
......................................................................


Patch Set 4:

I think that patch could be useful for 1.7.x, but I am not positive it's required as we haven't seen it reported yet. If you want to break it up, that works for me.


-- 
To view, visit http://gerrit.cloudera.org:8080/9848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a
Gerrit-Change-Number: 9848
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 03 Apr 2018 01:20:42 +0000
Gerrit-HasComments: No