You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2019/10/02 23:24:42 UTC

[kudu-CR] [WIP] KUDU-1938 Performance tuning pt 2

Hello Adar Dembo, Grant Henke,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: [WIP] KUDU-1938 Performance tuning pt 2
......................................................................

[WIP] KUDU-1938 Performance tuning pt 2

Adds Intel Intrinsics (up to SSE4.2) to speed up the processing of UTF8
character counting in the case of ASCII-only chunks by doubling the
chunk size in a single pass from 64 to 128 bits.

Attilas-MacBook-Pro:debug charger$ ./bin/char_util-test
[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from CharUtilTest
[ RUN      ] CharUtilTest.CorrectnessTestUtf8
[       OK ] CharUtilTest.CorrectnessTestUtf8 (1 ms)
[ RUN      ] CharUtilTest.CorrectnessTestAscii
[       OK ] CharUtilTest.CorrectnessTestAscii (1 ms)
[ RUN      ] CharUtilTest.StressTestUtf8
[       OK ] CharUtilTest.StressTestUtf8 (4496 ms)
[ RUN      ] CharUtilTest.StressTestAscii
[       OK ] CharUtilTest.StressTestAscii (650 ms)
[----------] 4 tests from CharUtilTest (5148 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test case ran. (5149 ms total)
[  PASSED  ] 4 tests.

Change-Id: I9a491157dd5c8b4815030bbda921a0afc0bafd28
---
M src/kudu/util/char_util.cc
1 file changed, 9 insertions(+), 6 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9a491157dd5c8b4815030bbda921a0afc0bafd28
Gerrit-Change-Number: 14354
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>

[kudu-CR] KUDU-1938 Make UTF-8 truncation faster pt 2

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14354 )

Change subject: KUDU-1938 Make UTF-8 truncation faster pt 2
......................................................................


Patch Set 14: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14354/11//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14354/11//COMMIT_MSG@22
PS11, Line 22: [ RUN      ] CharUtilTest.StressTestUtf8
             : [       OK ] CharUtilTest.StressTestUtf8 (9285 ms)
> Out of curiosity I checked what happens if we can use AVX instructions, her
Well, let's not lose sight of the broader context: this functionality micro-optimizes writing VARCHAR data to Kudu. Given that we'd expect most folks to use STRING and not VARCHAR, perhaps we should timebox this endeavour. Personally I'd optimize for UTF-8 over ASCII but it probably doesn't matter much either way.


http://gerrit.cloudera.org:8080/#/c/14354/14/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:

PS14: 
New indentation doesn't look right; the bodies of the if/else-if shouldn't align with the condition, they should align with two-chars-after-the-brace.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a491157dd5c8b4815030bbda921a0afc0bafd28
Gerrit-Change-Number: 14354
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 13 Nov 2019 06:55:04 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1938 Make UTF-8 truncation faster pt 2

Posted by "Attila Bukor (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/14354

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

Change subject: KUDU-1938 Make UTF-8 truncation faster pt 2
......................................................................

KUDU-1938 Make UTF-8 truncation faster pt 2

Adds Intel Intrinsics (up to SSE4.2) to speed up the processing of UTF8
character counting in the case of ASCII-only chunks (fast path) by
doubling the chunk size in a single pass from 64 to 128 bits.

Before:

[ RUN      ] CharUtilTest.StressTestUtf8
[       OK ] CharUtilTest.StressTestUtf8 (7746 ms)
[ RUN      ] CharUtilTest.StressTestAscii
[       OK ] CharUtilTest.StressTestAscii (1028 ms)

After:

[ RUN      ] CharUtilTest.StressTestUtf8
[       OK ] CharUtilTest.StressTestUtf8 (10599 ms)
[ RUN      ] CharUtilTest.StressTestAscii
[       OK ] CharUtilTest.StressTestAscii (676 ms)

Change-Id: I9a491157dd5c8b4815030bbda921a0afc0bafd28
---
M src/kudu/util/char_util-test.cc
M src/kudu/util/char_util.cc
2 files changed, 28 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/14354/11
-- 
To view, visit http://gerrit.cloudera.org:8080/14354
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a491157dd5c8b4815030bbda921a0afc0bafd28
Gerrit-Change-Number: 14354
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-1938 Make UTF-8 truncation faster pt 2

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/14354 )

Change subject: KUDU-1938 Make UTF-8 truncation faster pt 2
......................................................................


Patch Set 13:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/14354/11//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14354/11//COMMIT_MSG@22
PS11, Line 22: [ RUN      ] CharUtilTest.StressTestUtf8
             : [       OK ] CharUtilTest.StressTestUtf8 (9285 ms)
> It'll be really hard to provide guidance on when to enable that optimizatio
yeah maybe allow setting it on SetSliceCopy? I tried having a fallback fast path the way you proposed as well, but it was unfortunately slower in both cases:

8 and 16 byte fast paths (aligned):

[ RUN      ] CharUtilTest.StressTestUtf8
[       OK ] CharUtilTest.StressTestUtf8 (11317 ms)
[ RUN      ] CharUtilTest.StressTestAscii
[       OK ] CharUtilTest.StressTestAscii (777 ms)

I believe this is due to the extra branches which I suspect was the case with my previous attempt at making the slow path faster (instead of counting everything that's not the first byte of an UTF8 character, check the first ones only and advance the pointer to the start of the next character based on the first byte).

Interestingly enough I tried again now without aligning to 8/16 bytes and it was faster this way even though aligned seemed better with the non-SSE instructions:

8 byte fast path only with (unaligned):

[ RUN      ] CharUtilTest.StressTestUtf8
[       OK ] CharUtilTest.StressTestUtf8 (7485 ms)
[ RUN      ] CharUtilTest.StressTestAscii
[       OK ] CharUtilTest.StressTestAscii (1160 ms)

16 byte fast path only (unaligned):

[ RUN      ] CharUtilTest.StressTestUtf8
[       OK ] CharUtilTest.StressTestUtf8 (11150 ms)
[ RUN      ] CharUtilTest.StressTestAscii
[       OK ] CharUtilTest.StressTestAscii (754 ms)

It seems unaligned two-lane fast path wins over all other cases though:

8 and 16 byte fast paths (unaligned):

[ RUN      ] CharUtilTest.StressTestUtf8
[       OK ] CharUtilTest.StressTestUtf8 (9285 ms)
[ RUN      ] CharUtilTest.StressTestAscii
[       OK ] CharUtilTest.StressTestAscii (708 ms)


http://gerrit.cloudera.org:8080/#/c/14354/11/src/kudu/util/char_util-test.cc
File src/kudu/util/char_util-test.cc:

http://gerrit.cloudera.org:8080/#/c/14354/11/src/kudu/util/char_util-test.cc@97
PS11, Line 97: TEST_F(CharUtilTest, CorrectnessTestUtf8AndAscii) {
             :   Slice result;
             :   Slice data = "ááááááááááááááááááááááááááááááááaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> Can you do it in one line?
Done


http://gerrit.cloudera.org:8080/#/c/14354/9/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:

PS9: 
> Bear in mind that we already have CheckCPUFlags() to verify that the CPU su
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a491157dd5c8b4815030bbda921a0afc0bafd28
Gerrit-Change-Number: 14354
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 12 Nov 2019 12:29:43 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1938 Make UTF-8 truncation faster pt 2

Posted by "Attila Bukor (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/14354

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

Change subject: KUDU-1938 Make UTF-8 truncation faster pt 2
......................................................................

KUDU-1938 Make UTF-8 truncation faster pt 2

Adds Intel Intrinsics (up to SSE4.2) to speed up the processing of UTF8
character counting in the case of ASCII-only chunks (fast path) by
doubling the chunk size in a single pass from 64 to 128 bits.

Before:

[ RUN      ] CharUtilTest.StressTestUtf8
[       OK ] CharUtilTest.StressTestUtf8 (7746 ms)
[ RUN      ] CharUtilTest.StressTestAscii
[       OK ] CharUtilTest.StressTestAscii (1028 ms)

After:

[ RUN      ] CharUtilTest.StressTestUtf8
[       OK ] CharUtilTest.StressTestUtf8 (9285 ms)
[ RUN      ] CharUtilTest.StressTestAscii
[       OK ] CharUtilTest.StressTestAscii (708 ms)

Change-Id: I9a491157dd5c8b4815030bbda921a0afc0bafd28
---
M src/kudu/util/char_util-test.cc
M src/kudu/util/char_util.cc
2 files changed, 37 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/14354/15
-- 
To view, visit http://gerrit.cloudera.org:8080/14354
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a491157dd5c8b4815030bbda921a0afc0bafd28
Gerrit-Change-Number: 14354
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-1938 Make UTF-8 truncation faster pt 2

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/14354 )

Change subject: KUDU-1938 Make UTF-8 truncation faster pt 2
......................................................................


Patch Set 12:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/14354/11//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14354/11//COMMIT_MSG@22
PS11, Line 22: [ RUN      ] CharUtilTest.StressTestUtf8
             : [       OK ] CharUtilTest.StressTestUtf8 (10599 ms)
> Looks like this got slower? Why?
I think it's due to the fact that this way we can only fast runs of 16 ASCII characters instead of 8 characters. I was wondering if I should make ASCII optimization optional via a flag. What do you think?


http://gerrit.cloudera.org:8080/#/c/14354/11/src/kudu/util/char_util-test.cc
File src/kudu/util/char_util-test.cc:

http://gerrit.cloudera.org:8080/#/c/14354/11/src/kudu/util/char_util-test.cc@97
PS11, Line 97:   Slice data;
             : 
             :   data = "ááááááááááááááááááááááááááááááááaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
> Combine?
I'm not sure what you mean.


http://gerrit.cloudera.org:8080/#/c/14354/9/src/kudu/util/char_util.cc
File src/kudu/util/char_util.cc:

PS9: 
> Think you missed this.
I'm still thinking how to do this exactly. This will be used by the clients instead of the server-side and AFAIK we don't have any CPU restrictions there - if someone wants to write to Kudu through the C++ client from a Raspberry Pi or some embedded system, we should support it as best as we reasonably can.

So I was thinking maybe putting the whole thing behind #if __SSE4_1__ so that we don't even compile the part if the target -march doesn't support it and then do a has_sse41 check just in case and if it doesn't, throw an exception?

Also, if this is not part of the Kudu server targets/binaries and it can be easily disabled with setting the appropriate -march, do we still need to limit ourselves to SSE4.2 or can we use AVX* behind #if?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a491157dd5c8b4815030bbda921a0afc0bafd28
Gerrit-Change-Number: 14354
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 12 Nov 2019 08:31:03 +0000
Gerrit-HasComments: Yes