Optimize block sort by solotzg · Pull Request #5908 · pingcap/tiflash

solotzg · 2022-09-16T03:23:57Z

What problem does this PR solve?

Issue Number: ref #5294

What is changed and how it works?

optimize when number of sort key is 2 and type of sort key is one of
- UInt64
- Int64
- StringBinPadding
- StringBinNoPadding
- StringWithCollatorGeneric
we can optimize the case that number of sort key is 3 later if necessary (there will be a lot of template instantiations)

Benchmark

ENV

tpch-100
1 tiflash
limit cpu up to 2000%
original commit: 6d0cbc8

SQL

EXPLAIN analyze SELECT ROW_NUMBER() OVER w1 FROM PART window w1 AS (PARTITION BY P_MFGR order by P_SIZE);

sort key: STR(utf8 collator),UINT64

Time(s)	Original	Optimized		Improvement
	8.16	7.18
	8.22	6.94
	8.29	7.35
	8.34	7.04
	8.24	7.37		AVG(Original) / AVG(Optimized) - 1.0
AVG	8.25	7.176	Optimized : Original	14.97%

SQL

EXPLAIN analyze SELECT ROW_NUMBER() OVER w1 FROM PART window w1 AS (PARTITION BY P_MFGR ORDER BY p_name);

sort key: STR(utf8 collator),STR(utf8 collator)

Time(s)	Original	Optimized		Improvement
	18.3	16.34
	18.27	16.62
	18.7	16.57
	18.57	16.52
	18.55	16.63		AVG(Original) / AVG(Optimized) - 1.0
AVG	18.478	16.536	Optimized : Original	11.74%

SQL

EXPLAIN analyze SELECT ROW_NUMBER() OVER w1 FROM PART window w1 AS (PARTITION BY p_name ORDER BY p_partkey);

sort key: INT64,STR(utf8 collator)
costs of block sort are about 30%

Time(s)	Original	Optimized		Improvement
	9.71	9.28
	9.62	9.41
	9.76	9.25
	9.73	9.26
	9.67	9.28		AVG(Original) / AVG(Optimized) - 1.0
AVG	9.698	9.296	Optimized : Original	4.32%

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

Signed-off-by: Zhigao Tong <tongzhigao@pingcap.com>

ti-chi-bot · 2022-09-16T03:23:58Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

LittleFall
windtalker

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

solotzg · 2022-09-16T03:36:51Z

/run-all-tests

solotzg · 2022-09-16T03:46:08Z

/run-unit-test

sre-bot · 2022-09-16T04:16:07Z

Coverage for changed files

Filename                                Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/tests/gtest_topn_executor.cpp         118                22    81.36%           6                 0   100.00%         235                 2    99.15%          54                13    75.93%
Interpreters/sortBlock.cpp                  200                23    88.50%          28                 0   100.00%         305                 8    97.38%         116                11    90.52%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                       318                45    85.85%          34                 0   100.00%         540                10    98.15%         170                24    85.88%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18728      8128             56.60%    216616  83550        61.43%

full coverage report (for internal network access only)

Signed-off-by: Zhigao Tong <tongzhigao@pingcap.com>

solotzg · 2022-09-18T13:10:32Z

/run-all-tests

solotzg · 2022-09-19T02:01:13Z

/run-all-tests

sre-bot · 2022-09-19T02:20:30Z

Coverage for changed files

Filename                                         Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
dbms/src/Flash/tests/gtest_topn_executor.cpp          96                22    77.08%           6                 0   100.00%         228                 2    99.12%          50                13    74.00%
dbms/src/Interpreters/sortBlock.cpp                  200                23    88.50%          28                 0   100.00%         307                 8    97.39%         116                11    90.52%
libs/libcommon/include/common/avx2_strstr.h          149                17    88.59%          14                 0   100.00%         236                38    83.90%          82                 3    96.34%
libs/libcommon/include/common/sse2_memcpy.h           46                 0   100.00%           1                 0   100.00%         111                27    75.68%          18                 1    94.44%
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                491                62    87.37%          49                 0   100.00%         882                75    91.50%         266                28    89.47%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18885      8119             57.01%    219788  83654        61.94%

full coverage report (for internal network access only)

dbms/src/Flash/tests/gtest_topn_executor.cpp

solotzg · 2022-09-19T06:49:00Z

/run-all-tests

sre-bot · 2022-09-19T07:12:57Z

Coverage for changed files

Filename                                             Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
dbms/src/Interpreters/sortBlock.cpp                      200                23    88.50%          28                 0   100.00%         307                 8    97.39%         116                11    90.52%
dbms/src/Interpreters/tests/gtest_block_sort.cpp          46                 7    84.78%           1                 0   100.00%          99                 0   100.00%          28                 4    85.71%
libs/libcommon/include/common/avx2_strstr.h              149                17    88.59%          14                 0   100.00%         236                38    83.90%          82                 3    96.34%
libs/libcommon/include/common/sse2_memcpy.h               46                 0   100.00%           1                 0   100.00%         111                27    75.68%          18                 1    94.44%
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                    441                47    89.34%          44                 0   100.00%         753                73    90.31%         244                19    92.21%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18885      8120             57.00%    219788  83685        61.92%

full coverage report (for internal network access only)

solotzg · 2022-09-21T09:36:26Z

It seems that the optimization is all from move branches out of loops and make branches static?

from the offline discussion, we can materialize the collator look-up column, to decrease the collator look-up times from O(nlogn) to O(n)

It may also be an optional way if the overhead about materialization is small.

LittleFall

consider add a FastPathPermutationSort<1>, which is commoner than 2 columns

windtalker · 2022-09-22T05:39:33Z

dbms/src/Interpreters/sortBlock.cpp

+
+// only for uint64, int64, string
+template <typename ColumnCmpA, typename ColumnCmpB>
+struct MultiColumnSortFastPath


MultiColumn is is actullay two column?

Yes, if use 3 columns, there will be too many template instantiations. We may need to optimize later if necessary.

windtalker · 2022-09-22T06:01:47Z

dbms/src/Interpreters/sortBlock.cpp

+    }
+};
+
+struct CollatorDesc : boost::noncopyable


Why use CollatorDesc as the name, maybe use FastSortDesc instead?

windtalker · 2022-09-22T07:32:46Z

dbms/src/Interpreters/sortBlock.cpp

 struct PartialSortingLessWithCollation
 {
-    const ColumnsWithSortDescriptions & columns;
+    const CollatorDesc & collator_desc;


Using CollatorDesc instead of ColumnsWithSortDescriptions seems does not make too much difference?

PartialSortingLessWithCollation uses member variables has_collation and need_collations of CollatorDesc

solotzg · 2022-09-22T09:13:16Z

consider add a FastPathPermutationSort<1>, which is commoner than 2 columns

We already have such optimization :

tiflash/dbms/src/Interpreters/sortBlock.cpp

Lines 123 to 141 in 3674439

    
           if (description.size() == 1) 
        
           { 
        
               bool reverse = description[0].direction == -1; 
        
               const IColumn * column = !description[0].column_name.empty() 
        
                   ? block.getByName(description[0].column_name).column.get() 
        
                   : block.safeGetByPosition(description[0].column_number).column.get(); 
        
               IColumn::Permutation perm; 
        
               if (needCollation(column, description[0])) 
        
                   column->getPermutation(*description[0].collator, reverse, limit, description[0].nulls_direction, perm); 
        
               else 
        
                   column->getPermutation(reverse, limit, description[0].nulls_direction, perm); 
        
               size_t columns = block.columns(); 
        
               for (size_t i = 0; i < columns; ++i) 
        
                   block.safeGetByPosition(i).column = block.safeGetByPosition(i).column->permute(perm, limit); 
        
           } 
        
           else

…ation

windtalker

others LGTM

windtalker · 2022-09-23T06:05:45Z

dbms/src/Interpreters/sortBlock.cpp

 struct PartialSortingLessWithCollation
 {
-    const ColumnsWithSortDescriptions & columns;
+    const FastSortDesc & collator_desc;


The variable name should also be modified.

Signed-off-by: Zhigao Tong <tongzhigao@pingcap.com>

solotzg · 2022-09-23T06:15:45Z

/hold

solotzg · 2022-09-23T06:15:51Z

/merge

ti-chi-bot · 2022-09-23T06:15:53Z

@solotzg: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-09-23T06:15:54Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: f117304

solotzg · 2022-09-23T06:26:24Z

/rebuild

solotzg · 2022-09-23T06:27:28Z

/run-all-tests

solotzg · 2022-09-23T06:42:05Z

/rebuild

solotzg · 2022-09-23T06:42:16Z

/merge

ti-chi-bot · 2022-09-23T06:42:17Z

@solotzg: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

solotzg · 2022-09-23T06:48:17Z

/run-integration-test

sre-bot · 2022-09-23T07:09:45Z

Coverage for changed files

Filename                                             Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
dbms/src/Interpreters/sortBlock.cpp                      200                24    88.00%          28                 0   100.00%         307                 8    97.39%         116                12    89.66%
dbms/src/Interpreters/tests/gtest_block_sort.cpp          46                 7    84.78%           1                 0   100.00%          99                 0   100.00%          28                 4    85.71%
libs/libcommon/include/common/avx2_strstr.h              149                17    88.59%          14                 0   100.00%         236                38    83.90%          82                 3    96.34%
libs/libcommon/include/common/sse2_memcpy.h               46                 0   100.00%           1                 0   100.00%         111                27    75.68%          18                 0   100.00%
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                    441                48    89.12%          44                 0   100.00%         753                73    90.31%         244                19    92.21%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18343      7506             59.08%    214602  77144        64.05%

full coverage report (for internal network access only)

solotzg · 2022-09-23T07:15:45Z

/unhold

sre-bot · 2022-09-23T07:46:56Z

Coverage for changed files

Filename                                             Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
dbms/src/Interpreters/sortBlock.cpp                      200                24    88.00%          28                 0   100.00%         307                 8    97.39%         116                12    89.66%
dbms/src/Interpreters/tests/gtest_block_sort.cpp          46                 7    84.78%           1                 0   100.00%          99                 0   100.00%          28                 4    85.71%
libs/libcommon/include/common/avx2_strstr.h              149                17    88.59%          14                 0   100.00%         236                38    83.90%          82                 3    96.34%
libs/libcommon/include/common/sse2_memcpy.h               46                 0   100.00%           1                 0   100.00%         111                27    75.68%          18                 0   100.00%
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                    441                48    89.12%          44                 0   100.00%         753                73    90.31%         244                19    92.21%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18343      7504             59.09%    214602  77121        64.06%

full coverage report (for internal network access only)

This reverts commit 4f66bc3.

solotzg added 2 commits September 16, 2022 10:14

optimize block sort by multi column

4db0c10

Signed-off-by: Zhigao Tong <tongzhigao@pingcap.com>

refine & optimize

3ec66f0

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 16, 2022

solotzg added type/enhancement The issue or PR belongs to an enhancement. type/performance and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 16, 2022

solotzg requested review from LittleFall, windtalker and zanmato1984 September 16, 2022 03:26

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 16, 2022

solotzg mentioned this pull request Sep 16, 2022

Improve the performance of new collation releated function/executor #5294

Closed

9 tasks

solotzg added 4 commits September 16, 2022 12:25

fix typo

79b2738

Merge branch 'master' into optimize-sort-collation

981f945

optimize code

40d007a

refine

d7a5b3a

Signed-off-by: Zhigao Tong <tongzhigao@pingcap.com>

SeaRise reviewed Sep 19, 2022

View reviewed changes

dbms/src/Flash/tests/gtest_topn_executor.cpp Outdated Show resolved Hide resolved

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 19, 2022

refine gtests

d6683c1

solotzg force-pushed the optimize-sort-collation branch from fc0be79 to d6683c1 Compare September 19, 2022 06:41

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 21, 2022

LittleFall reviewed Sep 22, 2022

View reviewed changes

windtalker reviewed Sep 22, 2022

View reviewed changes

rename CollatorDesc to FastSortDesc

35c4c3e

Merge remote-tracking branch 'pingcap/master' into optimize-sort-coll…

f2558b1

…ation

windtalker approved these changes Sep 23, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 23, 2022

rename

f117304

Signed-off-by: Zhigao Tong <tongzhigao@pingcap.com>

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 23, 2022

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2022

ti-chi-bot merged commit 4f66bc3 into pingcap:master Sep 23, 2022

solotzg deleted the optimize-sort-collation branch September 23, 2022 07:18

solotzg added a commit to solotzg/tiflash that referenced this pull request Sep 23, 2022

Revert "Optimize block sort (pingcap#5908)"

5718f58

This reverts commit 4f66bc3.

Conversation

solotzg commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Benchmark

Check List

Release note

Uh oh!

ti-chi-bot commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

solotzg commented Sep 16, 2022

Uh oh!

solotzg commented Sep 16, 2022

Uh oh!

sre-bot commented Sep 16, 2022

Coverage for changed files

Coverage summary

Uh oh!

solotzg commented Sep 18, 2022

Uh oh!

solotzg commented Sep 19, 2022

Uh oh!

sre-bot commented Sep 19, 2022

Coverage for changed files

Coverage summary

Uh oh!

Uh oh!

solotzg commented Sep 19, 2022

Uh oh!

sre-bot commented Sep 19, 2022

Coverage for changed files

Coverage summary

Uh oh!

solotzg commented Sep 21, 2022

Uh oh!

LittleFall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solotzg commented Sep 22, 2022

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solotzg commented Sep 23, 2022

Uh oh!

solotzg commented Sep 23, 2022

Uh oh!

ti-chi-bot commented Sep 23, 2022

Uh oh!

ti-chi-bot commented Sep 23, 2022

Uh oh!

solotzg commented Sep 23, 2022

Uh oh!

solotzg commented Sep 23, 2022

Uh oh!

solotzg commented Sep 23, 2022

Uh oh!

solotzg commented Sep 23, 2022

Uh oh!

ti-chi-bot commented Sep 23, 2022

solotzg commented Sep 16, 2022 •

edited

Loading

ti-chi-bot commented Sep 16, 2022 •

edited

Loading