Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: do del column clean read optimization for fast mode #5470

Merged

Conversation

hongyunyan
Copy link
Contributor

@hongyunyan hongyunyan commented Jul 26, 2022

What problem does this PR solve?

Issue Number: ref #5252

Problem Summary:
For the query in fast mode, we always read the del_mark column, and then do filter with del_mark = 1. While for the packs that all rows' del_mark = 0, we don't need to read their del_mark column, and also don't need to do the filter with del_mark. Thus we add these optimization to cut down IO.

Now, some code is just for benchmark test(bench_del_optimization). After the review, I will delete the related codes.

What is changed and how it works?

  1. fix the min-max index calculation for del column.
  2. add the is_delete in PackStat to store the delete rows in a pack.
  3. use is_delete to make read optimization on del column in fast mode.

performance test:
I run the bench_del_optimization benchmark(to compare the performance with/without del optimization to read a dmfile with no delete rows) , the performance is
image

Besides,I run the dtworkload, and the total performance is almost the same.

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 26, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • JaySon-Huang
  • lidezhu

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.

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

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 26, 2022
@hongyunyan
Copy link
Contributor Author

/run-all-tests

@hongyunyan hongyunyan changed the title feat: do del column clean read optimization for fast mode WIP: feat: do del column clean read optimization for fast mode Jul 26, 2022
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2022
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 28, 2022
@ti-chi-bot 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 Aug 12, 2022
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2022
@hongyunyan hongyunyan changed the title WIP: feat: do del column clean read optimization for fast mode feat: do del column clean read optimization for fast mode Aug 15, 2022
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 15, 2022
@hongyunyan
Copy link
Contributor Author

/run-all-tests

@hongyunyan
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Aug 24, 2022

Coverage for changed files

Filename                                                                   Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Server/DTTool/DTToolMigrate.cpp                                                245                66    73.06%          11                 1    90.91%         283               154    45.58%         116                68    41.38%
Storages/DeltaMerge/DMDecoratorStreams.h                                        54                 8    85.19%          13                 6    53.85%         106                27    74.53%          22                 3    86.36%
Storages/DeltaMerge/DMSegmentThreadInputStream.h                                84                 3    96.43%           6                 0   100.00%          87                 2    97.70%          26                 7    73.08%
Storages/DeltaMerge/DMVersionFilterBlockInputStream.cpp                        113                11    90.27%           5                 0   100.00%         332                 5    98.49%          82                10    87.80%
Storages/DeltaMerge/DMVersionFilterBlockInputStream.h                           87                11    87.36%          12                 1    91.67%         110                 1    99.09%          46                13    71.74%
Storages/DeltaMerge/DeltaMergeStore.cpp                                       1675               586    65.01%          67                 5    92.54%        2138               523    75.54%         750               313    58.27%
Storages/DeltaMerge/DeltaMergeStore.h                                           41                10    75.61%          19                 2    89.47%         107                24    77.57%          42                 8    80.95%
Storages/DeltaMerge/File/DMFileBlockInputStream.cpp                             12                 2    83.33%           2                 0   100.00%          48                 6    87.50%          12                 3    75.00%
Storages/DeltaMerge/File/DMFileBlockInputStream.h                               20                 3    85.00%          16                 1    93.75%          66                 7    89.39%           4                 2    50.00%
Storages/DeltaMerge/File/DMFileReader.cpp                                      271                27    90.04%          15                 1    93.33%         507                64    87.38%         192                37    80.73%
Storages/DeltaMerge/File/DMFileReader.h                                         11                 2    81.82%           5                 2    60.00%          13                 6    53.85%           4                 0   100.00%
Storages/DeltaMerge/File/DMFileWriter.cpp                                      119                 7    94.12%          15                 0   100.00%         444                17    96.17%          78                 7    91.03%
Storages/DeltaMerge/File/DMFileWriter.h                                         24                 1    95.83%          14                 1    92.86%          37                 3    91.89%           6                 0   100.00%
Storages/DeltaMerge/Index/MinMaxIndex.cpp                                      494                89    81.98%          16                 2    87.50%         417                61    85.37%         250                93    62.80%
Storages/DeltaMerge/SSTFilesToBlockInputStream.cpp                             110               110     0.00%          14                14     0.00%         179               179     0.00%          58                58     0.00%
Storages/DeltaMerge/SSTFilesToBlockInputStream.h                                 4                 4     0.00%           4                 4     0.00%           4                 4     0.00%           0                 0         -
Storages/DeltaMerge/SSTFilesToDTFilesOutputStream.cpp                           97                97     0.00%           8                 8     0.00%         152               152     0.00%          40                40     0.00%
Storages/DeltaMerge/Segment.cpp                                                805               205    74.53%          47                 3    93.62%        1187               263    77.84%         296               144    51.35%
Storages/DeltaMerge/Segment.h                                                   38                 4    89.47%          23                 3    86.96%          32                 4    87.50%           4                 2    50.00%
Storages/DeltaMerge/StableValueSpace.cpp                                       113                23    79.65%          15                 1    93.33%         278                66    76.26%          72                24    66.67%
Storages/DeltaMerge/StableValueSpace.h                                          18                 9    50.00%          14                 5    64.29%          35                26    25.71%           4                 4     0.00%
Storages/DeltaMerge/tests/MultiSegmentTestUtil.h                                46                 2    95.65%           6                 0   100.00%          86                 0   100.00%          10                 2    80.00%
Storages/DeltaMerge/tests/gtest_dm_delta_merge_store_for_fast_mode.cpp         248                70    71.77%          22                 0   100.00%        1141                 5    99.56%         192                45    76.56%
Storages/DeltaMerge/tests/gtest_dm_file.cpp                                    396               140    64.65%          43                 2    95.35%        1058                 4    99.62%         164                75    54.27%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                         5125              1490    70.93%         412                62    84.95%        8847              1603    81.88%        2470               958    61.21%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18380      8364             54.49%    211703  86499        59.14%

full coverage report (for internal network access only)

Copy link
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

dbms/src/Storages/DeltaMerge/File/DMFileReader.cpp Outdated Show resolved Hide resolved
dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp Outdated Show resolved Hide resolved
@ti-chi-bot 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 Aug 24, 2022
@hongyunyan
Copy link
Contributor Author

/run-all-tests

@hongyunyan
Copy link
Contributor Author

/rebuild

@hongyunyan
Copy link
Contributor Author

/run-integration-test

@hongyunyan
Copy link
Contributor Author

/rebuild

@sre-bot
Copy link
Collaborator

sre-bot commented Aug 24, 2022

Coverage for changed files

Filename                                                                   Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Server/DTTool/DTToolMigrate.cpp                                                245                66    73.06%          11                 1    90.91%         283               154    45.58%         116                68    41.38%
Storages/DeltaMerge/DMDecoratorStreams.h                                        54                 7    87.04%          13                 6    53.85%         106                20    81.13%          22                 2    90.91%
Storages/DeltaMerge/DMVersionFilterBlockInputStream.cpp                        113                11    90.27%           5                 0   100.00%         332                 5    98.49%          82                10    87.80%
Storages/DeltaMerge/DMVersionFilterBlockInputStream.h                           87                11    87.36%          12                 1    91.67%         110                 1    99.09%          46                13    71.74%
Storages/DeltaMerge/File/DMFileBlockInputStream.cpp                             12                 2    83.33%           2                 0   100.00%          48                 6    87.50%          12                 3    75.00%
Storages/DeltaMerge/File/DMFileBlockInputStream.h                               20                 3    85.00%          16                 1    93.75%          66                 7    89.39%           4                 2    50.00%
Storages/DeltaMerge/File/DMFileReader.cpp                                      271                23    91.51%          15                 1    93.33%         514                58    88.72%         192                32    83.33%
Storages/DeltaMerge/File/DMFileReader.h                                         11                 2    81.82%           5                 2    60.00%          13                 6    53.85%           4                 0   100.00%
Storages/DeltaMerge/File/DMFileWriter.cpp                                      119                 7    94.12%          15                 0   100.00%         444                17    96.17%          78                 7    91.03%
Storages/DeltaMerge/File/DMFileWriter.h                                         24                 1    95.83%          14                 1    92.86%          37                 3    91.89%           6                 0   100.00%
Storages/DeltaMerge/Index/MinMaxIndex.cpp                                      494                89    81.98%          16                 2    87.50%         417                61    85.37%         250                93    62.80%
Storages/DeltaMerge/SSTFilesToBlockInputStream.cpp                             110               110     0.00%          14                14     0.00%         179               179     0.00%          58                58     0.00%
Storages/DeltaMerge/SSTFilesToBlockInputStream.h                                 4                 4     0.00%           4                 4     0.00%           4                 4     0.00%           0                 0         -
Storages/DeltaMerge/SSTFilesToDTFilesOutputStream.cpp                           97                97     0.00%           8                 8     0.00%         152               152     0.00%          40                40     0.00%
Storages/DeltaMerge/Segment.cpp                                                803               205    74.47%          47                 3    93.62%        1183               263    77.77%         294               143    51.36%
Storages/DeltaMerge/StableValueSpace.cpp                                       113                23    79.65%          15                 1    93.33%         278                66    76.26%          72                24    66.67%
Storages/DeltaMerge/StableValueSpace.h                                          18                 9    50.00%          14                 5    64.29%          35                26    25.71%           4                 4     0.00%
Storages/DeltaMerge/tests/gtest_dm_delta_merge_store_for_fast_mode.cpp         248                70    71.77%          22                 0   100.00%        1141                 5    99.56%         192                45    76.56%
Storages/DeltaMerge/tests/gtest_dm_file.cpp                                    396               140    64.65%          43                 2    95.35%        1058                 4    99.62%         164                75    54.27%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                         3239               880    72.83%         291                52    82.13%        6400              1037    83.80%        1636               619    62.16%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18380      8362             54.50%    211682  86470        59.15%

full coverage report (for internal network access only)

@hongyunyan
Copy link
Contributor Author

/run-integration-test

Copy link
Contributor

@lidezhu lidezhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 24, 2022
Copy link
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ti-chi-bot 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 Aug 24, 2022
@hongyunyan
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@hongyunyan: 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.

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
Copy link
Member

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

Commit hash: b08aba1

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 24, 2022
@ti-chi-bot
Copy link
Member

@hongyunyan: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

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.

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.

@sre-bot
Copy link
Collaborator

sre-bot commented Aug 24, 2022

Coverage for changed files

Filename                                                                   Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Server/DTTool/DTToolMigrate.cpp                                                245                66    73.06%          11                 1    90.91%         283               154    45.58%         116                68    41.38%
Storages/DeltaMerge/DMDecoratorStreams.h                                        54                 7    87.04%          13                 6    53.85%         106                20    81.13%          22                 2    90.91%
Storages/DeltaMerge/DMVersionFilterBlockInputStream.cpp                        113                11    90.27%           5                 0   100.00%         332                 5    98.49%          82                10    87.80%
Storages/DeltaMerge/DMVersionFilterBlockInputStream.h                           87                11    87.36%          12                 1    91.67%         110                 1    99.09%          46                 9    80.43%
Storages/DeltaMerge/File/DMFileBlockInputStream.cpp                             12                 2    83.33%           2                 0   100.00%          48                 6    87.50%          12                 3    75.00%
Storages/DeltaMerge/File/DMFileBlockInputStream.h                               20                 3    85.00%          16                 1    93.75%          66                 7    89.39%           4                 2    50.00%
Storages/DeltaMerge/File/DMFileReader.cpp                                      271                24    91.14%          15                 1    93.33%         514                58    88.72%         192                33    82.81%
Storages/DeltaMerge/File/DMFileReader.h                                         11                 2    81.82%           5                 2    60.00%          13                 6    53.85%           4                 0   100.00%
Storages/DeltaMerge/File/DMFileWriter.cpp                                      119                 7    94.12%          15                 0   100.00%         444                17    96.17%          78                 7    91.03%
Storages/DeltaMerge/File/DMFileWriter.h                                         24                 1    95.83%          14                 1    92.86%          37                 3    91.89%           6                 0   100.00%
Storages/DeltaMerge/Index/MinMaxIndex.cpp                                      494                89    81.98%          16                 2    87.50%         417                61    85.37%         250                93    62.80%
Storages/DeltaMerge/SSTFilesToBlockInputStream.cpp                             110               110     0.00%          14                14     0.00%         179               179     0.00%          58                58     0.00%
Storages/DeltaMerge/SSTFilesToBlockInputStream.h                                 4                 4     0.00%           4                 4     0.00%           4                 4     0.00%           0                 0         -
Storages/DeltaMerge/SSTFilesToDTFilesOutputStream.cpp                           97                97     0.00%           8                 8     0.00%         152               152     0.00%          40                40     0.00%
Storages/DeltaMerge/Segment.cpp                                                837               105    87.46%          47                 1    97.87%        1195               121    89.87%         308               110    64.29%
Storages/DeltaMerge/StableValueSpace.cpp                                       113                21    81.42%          15                 1    93.33%         278                46    83.45%          72                21    70.83%
Storages/DeltaMerge/StableValueSpace.h                                          18                 7    61.11%          14                 3    78.57%          35                24    31.43%           4                 4     0.00%
Storages/DeltaMerge/tests/gtest_dm_delta_merge_store_for_fast_mode.cpp         248                70    71.77%          22                 0   100.00%        1141                 5    99.56%         192                45    76.56%
Storages/DeltaMerge/tests/gtest_dm_file.cpp                                    396               140    64.65%          43                 2    95.35%        1058                 4    99.62%         164                75    54.27%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                         3273               777    76.26%         291                48    83.51%        6412               873    86.38%        1650               580    64.85%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18417      8330             54.77%    212905  85974        59.62%

full coverage report (for internal network access only)

@ti-chi-bot ti-chi-bot merged commit 79ba720 into pingcap:master Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants