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.

Sign up for GitHub

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

Jump to bottom

optimize memory usage of lock cf #4857

Closed
wants to merge 8 commits into from
Closed

optimize memory usage of lock cf #4857

wants to merge 8 commits into from

Conversation

lidezhu
Copy link
Contributor

@lidezhu lidezhu commented May 10, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: ref #4728

Problem Summary: For lock cf, we store both it's TiKVValue for serialization and DecodedLockCFValue for lock resolve which is redundant and consume a lot of memory.

What is changed and how it works?

Avoid store TiKVValue for lock cf and generating it on the fly.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  1. load tpch50;
  2. run 2. update lineitem set L_SUPPKEY=100000 limit 50000000;

Before optimize:
image
After optimize:
image
The memory consumption peak decreases about 10GB.

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 May 10, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • flowbehappy

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 do-not-merge/needs-linked-issue 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 May 10, 2022
@lidezhu lidezhu changed the title Avoid save TiKVValue for lock cf [DNM] Avoid save TiKVValue for lock cf May 10, 2022
@lidezhu lidezhu changed the title [DNM] Avoid save TiKVValue for lock cf [DNM] avoid save TiKVValue for lock cf May 10, 2022
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 10, 2022
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2022
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 27, 2022
@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 27, 2022

/run-all-tests

@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 27, 2022

/run-unit-test

7 similar comments
@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 27, 2022

/run-unit-test

@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 28, 2022

/run-unit-test

@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 28, 2022

/run-unit-test

@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 28, 2022

/run-unit-test

@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 28, 2022

/run-unit-test

@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 28, 2022

/run-unit-test

@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 28, 2022

/run-unit-test

@sre-bot
Copy link
Collaborator

sre-bot commented Jun 28, 2022

Coverage for changed files

Filename                          Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
RegionCFDataBase.cpp                  186                23    87.63%          27                 1    96.30%         299                28    90.64%         108                28    74.07%
RegionCFDataBase.h                      1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
RegionCFDataTrait.h                    12                 2    83.33%           9                 2    77.78%          32                 6    81.25%           2                 0   100.00%
RegionData.cpp                         65                 8    87.69%          20                 2    90.00%         171                18    89.47%          50                16    68.00%
SerializationHelper.h                  32                 5    84.38%           4                 0   100.00%          34                 3    91.18%           8                 4    50.00%
TiKVRecordFormat.h                    105                10    90.48%          37                 1    97.30%         278                18    93.53%          50                 5    90.00%
tests/gtest_tikv_keyvalue.cpp        3015               729    75.82%           9                 4    55.56%         470                44    90.64%         984               519    47.26%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                3416               777    77.25%         107                10    90.65%        1285               117    90.89%        1202               572    52.41%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18388      9658             47.48%    206794  96754        53.21%

full coverage report (for internal network access only)

@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 28, 2022

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Jun 28, 2022

Coverage for changed files

Filename                          Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
RegionCFDataBase.cpp                  114                15    86.84%          27                 1    96.30%         292                28    90.41%          84                16    80.95%
RegionCFDataBase.h                      1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
RegionCFDataTrait.h                    12                 2    83.33%           9                 2    77.78%          32                 6    81.25%           2                 0   100.00%
RegionData.cpp                         65                 8    87.69%          20                 2    90.00%         171                18    89.47%          50                16    68.00%
TiKVRecordFormat.h                    105                10    90.48%          37                 1    97.30%         278                18    93.53%          50                 5    90.00%
tests/gtest_tikv_keyvalue.cpp        3015               729    75.82%           9                 4    55.56%         470                44    90.64%         984               519    47.26%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                3312               764    76.93%         103                10    90.29%        1244               114    90.84%        1170               556    52.48%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18388      9659             47.47%    206785  96749        53.21%

full coverage report (for internal network access only)

@lidezhu lidezhu changed the title [DNM] avoid save TiKVValue for lock cf optimize the memory usage of lock cf Jun 28, 2022
@lidezhu lidezhu changed the title optimize the memory usage of lock cf optimize memory usage of lock cf Jun 28, 2022
@lidezhu
Copy link
Contributor Author

lidezhu commented Jun 29, 2022

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Jun 29, 2022

Coverage for changed files

Filename                          Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
RegionCFDataBase.cpp                  114                15    86.84%          27                 1    96.30%         292                28    90.41%          84                16    80.95%
RegionCFDataBase.h                      1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
RegionCFDataTrait.h                    12                 2    83.33%           9                 2    77.78%          32                 6    81.25%           2                 0   100.00%
RegionData.cpp                         65                 8    87.69%          20                 2    90.00%         171                18    89.47%          50                16    68.00%
TiKVRecordFormat.h                    105                10    90.48%          37                 1    97.30%         278                18    93.53%          50                 5    90.00%
tests/gtest_tikv_keyvalue.cpp        3015               729    75.82%           9                 4    55.56%         470                44    90.64%         984               519    47.26%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                3312               764    76.93%         103                10    90.29%        1244               114    90.84%        1170               556    52.48%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18402      9635             47.64%    207064  96430        53.43%

full coverage report (for internal network access only)

@lidezhu lidezhu requested a review from flowbehappy June 29, 2022 08:33
@lidezhu lidezhu requested a review from CalvinNeo June 29, 2022 08:34
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 9, 2022
@CalvinNeo
Copy link
Member

/hold

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2022
@lidezhu
Copy link
Contributor Author

lidezhu commented Jul 11, 2022

The short value in lock cf may be used when adapting to cloud engine. So this optimization may be not applicable.

@flowbehappy flowbehappy mentioned this pull request Jul 25, 2022
Open
2 tasks
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2022
@ti-chi-bot
Copy link
Member

@lidezhu: PR needs rebase.

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 kubernetes/test-infra repository.

@lidezhu lidezhu closed this Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flowbehappy flowbehappy flowbehappy approved these changes

@CalvinNeo CalvinNeo Awaiting requested review from CalvinNeo

@JaySon-Huang JaySon-Huang Awaiting requested review from JaySon-Huang

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lidezhu @ti-chi-bot @sre-bot @CalvinNeo @flowbehappy @JaySon-Huang