Optimize string sort for default collation `UTF8MB4_BIN` by solotzg · Pull Request #5375 · pingcap/tiflash

solotzg · 2022-07-15T02:23:20Z

What problem does this PR solve?

Issue Number: close #5366 ref #5294

Problem Summary:

Wrong behavior when handle strcmp with default collation #5366: the original string contains '\0' at the end. To compare it under tidb collation, it must not be contained.

What is changed and how it works?

devirtualize function call when collation is UTF8MB4_BIN, which is the default one.

Tail '\0' doesn't be removed in those parts.

tiflash/dbms/src/Columns/ColumnString.h

Line 249 in 3674439

    
           auto sort_key = collator->sortKey(reinterpret_cast<const char *>(&chars[offset]), string_size, sort_key_container);

tiflash/dbms/src/Columns/ColumnString.h

Line 270 in 3674439

    
           auto sort_key = collator->sortKey(reinterpret_cast<const char *>(&chars[offset]), string_size, sort_key_container);

tiflash/dbms/src/Columns/ColumnString.cpp

Lines 317 to 318 in 432c153

    
           reinterpret_cast<const char *>(&parent.chars[parent.offsetAt(lhs)]), parent.sizeAt(lhs), 
        
           reinterpret_cast<const char *>(&parent.chars[parent.offsetAt(rhs)]), parent.sizeAt(rhs));

Benchmark

ENV:

tpch100
1 tiflash
limit cpu up to 200%
original commit: 97342db

SQL

select min(L_SHIPMODE) from lineitem;

Time(s)	Original	Optimized	NoCollation
	11.35	9.98	9.81
	11.33	9.98	9.88
	11.23	10	9.84
	11.22	9.82	9.73	NoCollation : Original	15.22%
	11.58	9.95	9.96	Optimized : Original	14.04%
AVG	11.342	9.946	9.844	NoCollation : Optimized	1.04%

SQL

select max(L_SHIPMODE) from lineitem;

Time(s)	Original	Optimized	NoCollation
	13.56	12.62	12.77
	13.74	12.51	12.27
	13.35	12.61	12.32
	13.63	12.63	12.45	NoCollation : Original	9.13%
	13.52	12.66	12.32	Optimized : Original	7.57%
AVG	13.56	12.606	12.426	NoCollation : Optimized	1.45%

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

ti-chi-bot · 2022-07-15T02:23:22Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

windtalker
zanmato1984

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.

dbms/src/Storages/Transaction/Collator.h

solotzg · 2022-07-21T01:54:26Z

/run-all-tests

sre-bot · 2022-07-21T02:18:31Z

Coverage for changed files

Filename                                               Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Columns/ColumnString.cpp                                   161                79    50.93%          24                15    37.50%         343               198    42.27%          88                43    51.14%
Columns/ColumnString.h                                      67                15    77.61%          32                 8    75.00%         167                49    70.66%          24                 6    75.00%
Functions/CollationOperatorOptimized.h                      26                20    23.08%           6                 4    33.33%         100                82    18.00%          16                10    37.50%
Interpreters/AggregationCommon.h                            65                43    33.85%          10                 8    20.00%         190               131    31.05%          46                30    34.78%
Interpreters/SetVariants.h                                   1                 1     0.00%           1                 1     0.00%           1                 1     0.00%           0                 0         -
Storages/Transaction/Collator.cpp                          194                52    73.20%          32                 5    84.38%         416               124    70.19%         168                70    58.33%
Storages/Transaction/CollatorUtils.h                        26                 8    69.23%           9                 2    77.78%          42                 8    80.95%          10                 3    70.00%
Storages/Transaction/tests/gtest_tidb_collator.cpp          69                 9    86.96%           6                 0   100.00%          48                 0   100.00%          26                 9    65.38%
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                      609               227    62.73%         120                43    64.17%        1307               593    54.63%         378               171    54.76%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18639      9397             49.58%    212524  94895        55.35%

full coverage report (for internal network access only)

dbms/src/Columns/ColumnString.cpp

windtalker

LGTM

zanmato1984

LGTM

solotzg · 2022-07-21T08:18:50Z

/merge

ti-chi-bot · 2022-07-21T08:18:51Z

@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-07-21T08:18:52Z

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

Details

Commit hash: 9359dbb

sre-bot · 2022-07-21T09:13:35Z

Coverage for changed files

Filename                                               Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Columns/ColumnString.cpp                                   161                80    50.31%          24                15    37.50%         343               200    41.69%          88                44    50.00%
Columns/ColumnString.h                                      67                13    80.60%          32                 6    81.25%         167                39    76.65%          24                 6    75.00%
Functions/CollationOperatorOptimized.h                      26                20    23.08%           6                 4    33.33%         100                82    18.00%          16                10    37.50%
Interpreters/AggregationCommon.h                            65                43    33.85%          10                 8    20.00%         190               131    31.05%          46                30    34.78%
Interpreters/SetVariants.h                                   1                 1     0.00%           1                 1     0.00%           1                 1     0.00%           0                 0         -
Storages/Transaction/Collator.cpp                          188                46    75.53%          32                 5    84.38%         411               118    71.29%         162                64    60.49%
Storages/Transaction/CollatorUtils.h                        26                 8    69.23%           9                 2    77.78%          42                 8    80.95%          10                 3    70.00%
Storages/Transaction/tests/gtest_tidb_collator.cpp          69                 9    86.96%           6                 0   100.00%          48                 0   100.00%          26                 9    65.38%
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                      603               220    63.52%         120                41    65.83%        1302               579    55.53%         372               166    55.38%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18780      9449             49.69%    213985  95464        55.39%

full coverage report (for internal network access only)

…gcap#5375)" This reverts commit d61c1df.

solotzg added 8 commits July 14, 2022 13:39

optimize string sort with collation

e938077

add test case

213371b

wip

0b1b842

wip

32e1efd

optimize sort key with collation

f8947c7

Merge branch 'backup' into optimize-sort

a0b0382

small fix

456c986

rollback

427850c

solotzg added the type/enhancement The issue or PR belongs to an enhancement. label Jul 15, 2022

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 15, 2022

rollback rename

0ac2526

solotzg added type/performance type/bugfix This PR fixes a bug. labels Jul 15, 2022

solotzg added 2 commits July 15, 2022 11:24

Fix gtests

7b43a8a

Fix gtests

800ebec

ti-chi-bot removed the do-not-merge/needs-triage-completed label Jul 15, 2022

format

a16d596

solotzg requested review from windtalker and zanmato1984 July 15, 2022 06:46

windtalker reviewed Jul 18, 2022

View reviewed changes

dbms/src/Storages/Transaction/Collator.h Outdated Show resolved Hide resolved

dbms/src/Storages/Transaction/Collator.h Outdated Show resolved Hide resolved

solotzg added 2 commits July 20, 2022 14:37

rollback modify

7d44c4d

rollback modify

9599ff2

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

solotzg requested a review from windtalker July 20, 2022 12:56

solotzg force-pushed the optimize-sort branch from f07cb9e to 9599ff2 Compare July 20, 2022 13:03

Fix compile

9119710

Fix

bac11cf

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2022

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

99d9eb0

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2022

pingcap deleted a comment from sre-bot Jul 21, 2022

windtalker reviewed Jul 21, 2022

View reviewed changes

dbms/src/Columns/ColumnString.cpp Show resolved Hide resolved

dbms/src/Columns/ColumnString.cpp Show resolved Hide resolved

windtalker approved these changes Jul 21, 2022

View reviewed changes

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

zanmato1984 approved these changes Jul 21, 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 Jul 21, 2022

Merge branch 'master' into optimize-sort

9359dbb

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

Merge branch 'master' into optimize-sort

424ab60

ti-chi-bot merged commit d61c1df into pingcap:master Jul 21, 2022

solotzg deleted the optimize-sort branch July 21, 2022 09:21

solotzg removed the type/bugfix This PR fixes a bug. label Jul 21, 2022

solotzg mentioned this pull request Aug 9, 2022

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

Closed

9 tasks

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

Revert "Optimize string sort for default collation UTF8MB4_BIN (pin…

4543c9b

…gcap#5375)" This reverts commit d61c1df.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize string sort for default collation `UTF8MB4_BIN`#5375

Optimize string sort for default collation `UTF8MB4_BIN`#5375
ti-chi-bot merged 19 commits intopingcap:masterfrom
solotzg:optimize-sort

solotzg commented Jul 15, 2022 •

edited

Loading

Uh oh!

ti-chi-bot commented Jul 15, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

solotzg commented Jul 21, 2022

Uh oh!

sre-bot commented Jul 21, 2022

Uh oh!

Uh oh!

Uh oh!

windtalker left a comment

Uh oh!

zanmato1984 left a comment

Uh oh!

solotzg commented Jul 21, 2022

Uh oh!

ti-chi-bot commented Jul 21, 2022

Uh oh!

ti-chi-bot commented Jul 21, 2022

Uh oh!

sre-bot commented Jul 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	reinterpret_cast<const char *>(&parent.chars[parent.offsetAt(lhs)]), parent.sizeAt(lhs),
	reinterpret_cast<const char *>(&parent.chars[parent.offsetAt(rhs)]), parent.sizeAt(rhs));

Conversation

solotzg commented Jul 15, 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 Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

solotzg commented Jul 21, 2022

Uh oh!

sre-bot commented Jul 21, 2022

Coverage for changed files

Coverage summary

Uh oh!

Uh oh!

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

zanmato1984 left a comment

Choose a reason for hiding this comment

Uh oh!

solotzg commented Jul 21, 2022

Uh oh!

ti-chi-bot commented Jul 21, 2022

Uh oh!

ti-chi-bot commented Jul 21, 2022

Uh oh!

sre-bot commented Jul 21, 2022

Coverage for changed files

Coverage summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

solotzg commented Jul 15, 2022 •

edited

Loading

ti-chi-bot commented Jul 15, 2022 •

edited

Loading