You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Jim Apple (Code Review)" <ge...@cloudera.org> on 2016/09/01 12:59:19 UTC
[Impala-ASF-CR] IMPALA-4058: SimdByteSwap::ByteSwap256 assumed memory was 16-byte aligned.
Jim Apple has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4205
Change subject: IMPALA-4058: SimdByteSwap::ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
IMPALA-4058: SimdByteSwap::ByteSwap256 assumed memory was 16-byte aligned.
This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.
Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
2 files changed, 20 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/1
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 5:
Clearly we were missing some basic test coverage. Do we have any other test gaps around this code?
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 2:
> > > Greetings, All.
> > > @Jim, considering the string reverse function is using the
> SIMDed
> > > Byteswap as internal implementation, do you thinkt it is okay
> to
> > > merge some test cases from this? https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
> > > Thank you :D
> >
> > What do you mean by "merge some test cases from"?
>
> Hi Jim. I have added some extra test cases for the function
> "reverse" in the file expr-test.cc: (line 1933-1946)
> https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
>
> If you don't mind, would you please take a look at that and then
> decide whether to add those test cases or not? Thank you :)
Done
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 6:
See https://gerrit.cloudera.org/#/c/4290/ for the benchmark
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Youwei Wang (Code Review)" <ge...@cloudera.org>.
Youwei Wang has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util.cc
File be/src/util/bit-util.cc:
Line 161: _mm_storeu_si128(reinterpret_cast<__m128i*>(dst + 16), part1);
> What's the perf impact? I assume we still benefit from specializing this r
Greetings All.
@Dan I have conducted the performance test using bswap-benchmark. There is no performance loss comparing the Impala-2809. And even I got slight performance improvement using these new code. (_mm_storeu_si128/_mm_loadu_si128)
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util.cc
File be/src/util/bit-util.cc:
Line 161: _mm_storeu_si128(reinterpret_cast<__m128i*>(dst + 16), part1);
> movdqa
What's the perf impact? I assume we still benefit from specializing this routine for AVX2?
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.
Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Reviewed-on: http://gerrit.cloudera.org:8080/4205
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exprs/expr-test.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 43 insertions(+), 13 deletions(-)
Approvals:
Internal Jenkins: Verified
Dan Hecht: Looks good to me, approved
Tim Armstrong: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 5:
How about adding an end-to-end case in expr.test (or somewhere) especially since this bug is more about how the environment is set up when invoking the routine so would be good to exercise it in the real environment (as opposed to the routine semantics).
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
I also extended the microbenchmark to test perf when loads and stores were actually misaligned (not just using the different instruction). AVX2 is still significantly faster than SSE. I'll post a CR with that benchmark change separately.
I0901 13:47:02.025802 4956 bswap-benchmark.cc:122] Machine Info: Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz
ByteSwap benchmark alignment=0:Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile
(relative) (relative) (relative)
---------------------------------------------------------------------------------------------------------
FastScalar 881 1.08e+03 1.14e+03 1X 1X 1X
SSSE3 9.33e+03 1.02e+04 1.03e+04 10.6X 9.46X 9.03X
AVX2 3.43e+04 3.78e+04 3.84e+04 38.9X 35.2X 33.7X
SIMD 3.27e+04 3.78e+04 3.83e+04 37.1X 35.2X 33.6X
ByteSwap benchmark alignment=1:Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile
(relative) (relative) (relative)
---------------------------------------------------------------------------------------------------------
FastScalar 1e+03 1.08e+03 1.15e+03 1X 1X 1X
SSSE3 8.67e+03 9.01e+03 9.11e+03 8.66X 8.31X 7.95X
AVX2 2.62e+04 2.75e+04 2.77e+04 26.2X 25.4X 24.2X
SIMD 2.56e+04 2.72e+04 2.75e+04 25.6X 25.1X 24X
http://gerrit.cloudera.org:8080/#/c/4205/5/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:
PS5, Line 116: o
or
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#4).
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.
Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/exprs/expr-test.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
3 files changed, 32 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/4
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 6:
> How about adding an end-to-end case in expr.test (or somewhere)
> especially since this bug is more about how the environment is set
> up when invoking the routine so would be good to exercise it in the
> real environment (as opposed to the routine semantics).
done
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:
Line 116: // IMPALA-4058: test that bswap_fptr works when it reads to o writes from memory with
> What was the result of this test before the fix? A crash?
Yes - a segfault.
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 1931: TestStringValue("reverse('abcdefg')", "gfedcba");
> I think let's just make it a separate test, the long functions are fine whe
Done in PS 5.
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util.cc
File be/src/util/bit-util.cc:
Line 161: _mm_storeu_si128(reinterpret_cast<__m128i*>(dst + 16), part1);
> What's the perf impact? I assume we still benefit from specializing this r
I ran the benchmark and the AVX2 version is still >3x faster than the SSSE3 version.
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#2).
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.
Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
2 files changed, 20 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/2
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:
Line 116: // IMPALA-4058: test that bswap_fptr works when it reads to o writes from memory with
What was the result of this test before the fix? A crash?
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 5:
We don't support parquet decimals wider than 16 bytes so I don't think the AVX2 routine would have kicked in there.
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 2:
> Greetings, All.
> @Jim, considering the string reverse function is using the SIMDed
> Byteswap as internal implementation, do you thinkt it is okay to
> merge some test cases from this? https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
> Thank you :D
What do you mean by "merge some test cases from"?
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 1931: {
Why the nested block? Should probably just be a separate function if we want to separate it.
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#5).
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.
Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/exprs/expr-test.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
3 files changed, 31 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/5
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util.cc
File be/src/util/bit-util.cc:
Line 161: _mm_storeu_si128(reinterpret_cast<__m128i*>(dst + 16), part1);
what instructions did the compiler emit before this change?
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Youwei Wang (Code Review)" <ge...@cloudera.org>.
Youwei Wang has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 2: Code-Review+1
Greetings, All.
@Jim, considering the string reverse function is using the SIMDed Byteswap as internal implementation, do you thinkt it is okay to merge some test cases from this? https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
Thank you :D
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 1931: {
> Just for scoping so MAX_LEN, to_reverse and reversed don't conflict with an
I think let's just make it a separate test, the long functions are fine when the tests are one-liners but when they have some setup, etc it gets hard to read (that's also all over the place in this file but I don't see a good reason to emulate it).
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#6).
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.
Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/exprs/expr-test.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 43 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/6
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 1931: {
> Why the nested block? Should probably just be a separate function if we wan
Just for scoping so MAX_LEN, to_reverse and reversed don't conflict with anything.
For better or worse, this file includes very long functions. I was trying to match that style without polluting the space of local variable names.
http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util.cc
File be/src/util/bit-util.cc:
Line 161: _mm_storeu_si128(reinterpret_cast<__m128i*>(dst + 16), part1);
> what instructions did the compiler emit before this change?
movdqa
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#3).
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.
Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/exprs/expr-test.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
3 files changed, 29 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/3
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Youwei Wang (Code Review)" <ge...@cloudera.org>.
Youwei Wang has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 2: -Code-Review
> > Greetings, All.
> > @Jim, considering the string reverse function is using the SIMDed
> > Byteswap as internal implementation, do you thinkt it is okay to
> > merge some test cases from this? https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
> > Thank you :D
>
> What do you mean by "merge some test cases from"?
Hi Jim. I have added some extra test cases for the function "reverse" in the file expr-test.cc: (line 1933-1946)
https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
If you don't mind, would you please take a look at that and then decide whether to add those test cases or not? Thank you :)
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <yo...@intel.com>
Gerrit-HasComments: No