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

PageStorage: Log down the gc time for external pages #5739

Merged
merged 13 commits into from
Aug 31, 2022
Merged

PageStorage: Log down the gc time for external pages #5739

merged 13 commits into from
Aug 31, 2022

Conversation

JaySon-Huang
Copy link
Contributor

@JaySon-Huang JaySon-Huang commented Aug 30, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: ref #5697

Problem Summary:
The GC total time in logging does not include the time costs in clean_external_page.

What is changed and how it works?

  • Add a struct GCTimeStatistics to handle logging
  • Add the time cost in cleanExternalPage into GC total time
  • Add ExternalIdsByNamespace to
    • Manage external ids by namespace
    • Shorten the GC time of cleaning external pages from 2500+ms to 420ms with about 5000 empty tables
# before
[2022/08/31 13:06:27.762 +08:00] [DEBUG] [PageStorageImpl.cpp:330] ["PageStorage:__global__.data GC finished without full gc. [total time=2514ms] [dump snapshots=0ms] [gc in mem entries=1ms] [blobstore remove entries=0ms] [blobstore get status=0ms] [get gc entries=0ms] [blobstore full gc=0ms] [gc apply=0ms] [external callbacks=4933] [external gc=2512ms] [get alive=2098.75ms] [scanner=408.65ms] [remover=4.90ms]"] [thread_id=163]

# after
[2022/08/31 13:10:22.715 +08:00] [DEBUG] [PageStorageImpl.cpp:330] ["PageStorage:__global__.data GC finished without full gc. [total time=427ms] [dump snapshots=0ms] [gc in mem entries=1ms] [blobstore remove entries=0ms] [blobstore get status=0ms] [get gc entries=0ms] [blobstore full gc=0ms] [gc apply=0ms] [external callbacks=4933] [external gc=425ms] [get alive=14.70ms] [scanner=407.08ms] [remover=3.83ms]"] [thread_id=136]

New limitation

We should not call shutdown for a table but later restore the DeltaMergeStore instance. It will remove the callbacks, also clean the external ids ptrs from PageStorage. After restoring the DeltaMergeStore from disk(without restoring the global PageStorage instance), it will run into removing all dtfiles from disk.
#5761

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 Aug 30, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • flowbehappy
  • 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 needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. needs-cherry-pick-release-6.2 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 Aug 30, 2022
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang JaySon-Huang mentioned this pull request Aug 30, 2022
Merged
12 tasks
@JaySon-Huang
Copy link
Contributor Author

/rebuild

@JaySon-Huang
Copy link
Contributor Author

/run-integration-test

@JaySon-Huang
Fix broken ut
cb7b6a8 
Signed-off-by: JaySon-Huang <[email protected]>
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Aug 31, 2022

Coverage for changed files

Filename                                             Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DeltaMerge/StoragePool.cpp                               268               117    56.34%          31                 9    70.97%         500               189    62.20%         138                71    48.55%
Page/V3/PageDirectory.cpp                                607                63    89.62%          31                 0   100.00%        1160               138    88.10%         444                78    82.43%
Page/V3/PageDirectory.h                                   27                 3    88.89%          27                 3    88.89%         110                10    90.91%           0                 0         -
Page/V3/PageDirectory/ExternalIdsByNamespace.cpp          14                 0   100.00%           4                 0   100.00%          41                 0   100.00%           8                 0   100.00%
Page/V3/PageDirectory/ExternalIdsByNamespace.h             1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Page/V3/PageDirectoryFactory.cpp                          55                 1    98.18%           6                 0   100.00%         127                 3    97.64%          50                 2    96.00%
Page/V3/PageStorageImpl.cpp                              138                24    82.61%          30                 4    86.67%         339                46    86.43%          70                15    78.57%
Page/V3/PageStorageImpl.h                                  9                 2    77.78%           9                 2    77.78%          23                 2    91.30%           0                 0         -
Page/V3/tests/gtest_page_directory.cpp                  1305               274    79.00%          65                 0   100.00%        2143                 7    99.67%         248               159    35.89%
Page/V3/tests/gtest_page_storage.cpp                     593               239    59.70%          51                 7    86.27%        1317               154    88.31%         216               118    45.37%
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                   3017               723    76.04%         255                25    90.20%        5761               549    90.47%        1174               443    62.27%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18488      8321             54.99%    213851  85872        59.84%

full coverage report (for internal network access only)

@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang
Copy link
Contributor Author

/cc @lidezhu @flowbehappy

@JaySon-Huang
fix naming
19ddf75 
Signed-off-by: JaySon-Huang <[email protected]>
@JaySon-Huang
Add comments; use unordered_map
6f0bd4a 
Signed-off-by: JaySon-Huang <[email protected]>
@sre-bot
Copy link
Collaborator

sre-bot commented Aug 31, 2022

Coverage for changed files

Filename                                             Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DeltaMerge/StoragePool.cpp                               268               117    56.34%          31                 9    70.97%         500               189    62.20%         138                71    48.55%
Page/V3/PageDirectory.cpp                                607                63    89.62%          31                 0   100.00%        1160               138    88.10%         444                79    82.21%
Page/V3/PageDirectory.h                                   27                 3    88.89%          27                 3    88.89%         110                10    90.91%           0                 0         -
Page/V3/PageDirectory/ExternalIdsByNamespace.cpp          14                 0   100.00%           4                 0   100.00%          41                 0   100.00%           8                 0   100.00%
Page/V3/PageDirectory/ExternalIdsByNamespace.h             1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Page/V3/PageDirectoryFactory.cpp                          55                 1    98.18%           6                 0   100.00%         127                 3    97.64%          50                 2    96.00%
Page/V3/PageStorageImpl.cpp                              138                24    82.61%          30                 4    86.67%         339                46    86.43%          70                15    78.57%
Page/V3/PageStorageImpl.h                                  9                 2    77.78%           9                 2    77.78%          23                 2    91.30%           0                 0         -
Page/V3/tests/gtest_page_directory.cpp                  1305               274    79.00%          65                 0   100.00%        2143                 7    99.67%         248               159    35.89%
Page/V3/tests/gtest_page_storage.cpp                     593               239    59.70%          51                 7    86.27%        1317               154    88.31%         216               118    45.37%
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                   3017               723    76.04%         255                25    90.20%        5761               549    90.47%        1174               444    62.18%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18488      8321             54.99%    213851  85888        59.84%

full coverage report (for internal network access only)

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 31, 2022
@JaySon-Huang
Address comment
d56fafa 
Signed-off-by: JaySon-Huang <[email protected]>
@JaySon-Huang
Remove duplicated code
633ceea 
Signed-off-by: JaySon-Huang <[email protected]>
lidezhu
lidezhu approved these changes Aug 31, 2022
View reviewed changes
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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 31, 2022
@JaySon-Huang
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@JaySon-Huang: 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: 633ceea

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

sre-bot commented Aug 31, 2022

Coverage for changed files

Filename                                             Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DeltaMerge/StoragePool.cpp                               268               117    56.34%          31                 9    70.97%         500               189    62.20%         138                71    48.55%
Page/V3/PageDirectory.cpp                                607                63    89.62%          31                 0   100.00%        1160               138    88.10%         444                79    82.21%
Page/V3/PageDirectory.h                                   27                 3    88.89%          27                 3    88.89%         110                10    90.91%           0                 0         -
Page/V3/PageDirectory/ExternalIdsByNamespace.cpp          14                 0   100.00%           4                 0   100.00%          39                 0   100.00%           8                 0   100.00%
Page/V3/PageDirectory/ExternalIdsByNamespace.h             1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Page/V3/PageDirectoryFactory.cpp                          55                 1    98.18%           6                 0   100.00%         127                 3    97.64%          50                 2    96.00%
Page/V3/PageStorageImpl.cpp                              138                24    82.61%          30                 4    86.67%         339                46    86.43%          70                15    78.57%
Page/V3/PageStorageImpl.h                                  9                 2    77.78%           9                 2    77.78%          23                 2    91.30%           0                 0         -
Page/V3/tests/gtest_page_directory.cpp                  1305               274    79.00%          65                 0   100.00%        2143                 7    99.67%         248               159    35.89%
Page/V3/tests/gtest_page_storage.cpp                     593               239    59.70%          51                 7    86.27%        1317               154    88.31%         216               118    45.37%
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                   3017               723    76.04%         255                25    90.20%        5759               549    90.47%        1174               444    62.18%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18488      8321             54.99%    213849  85849        59.86%

full coverage report (for internal network access only)

@JaySon-Huang
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@JaySon-Huang: 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.

@sre-bot
Copy link
Collaborator

sre-bot commented Aug 31, 2022

Coverage for changed files

Filename                                             Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DeltaMerge/StoragePool.cpp                               268               117    56.34%          31                 9    70.97%         500               189    62.20%         138                71    48.55%
Page/V3/PageDirectory.cpp                                607                63    89.62%          31                 0   100.00%        1160               138    88.10%         444                79    82.21%
Page/V3/PageDirectory.h                                   27                 3    88.89%          27                 3    88.89%         110                10    90.91%           0                 0         -
Page/V3/PageDirectory/ExternalIdsByNamespace.cpp          14                 0   100.00%           4                 0   100.00%          39                 0   100.00%           8                 0   100.00%
Page/V3/PageDirectory/ExternalIdsByNamespace.h             1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Page/V3/PageDirectoryFactory.cpp                          55                 1    98.18%           6                 0   100.00%         127                 3    97.64%          50                 2    96.00%
Page/V3/PageStorageImpl.cpp                              138                24    82.61%          30                 4    86.67%         339                46    86.43%          70                15    78.57%
Page/V3/PageStorageImpl.h                                  9                 2    77.78%           9                 2    77.78%          23                 2    91.30%           0                 0         -
Page/V3/tests/gtest_page_directory.cpp                  1305               274    79.00%          65                 0   100.00%        2143                 7    99.67%         248               159    35.89%
Page/V3/tests/gtest_page_storage.cpp                     593               239    59.70%          51                 7    86.27%        1317               154    88.31%         216               118    45.37%
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                   3017               723    76.04%         255                25    90.20%        5759               549    90.47%        1174               444    62.18%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18488      8321             54.99%    213849  85888        59.84%

full coverage report (for internal network access only)

@ti-chi-bot ti-chi-bot merged commit 6262e0a into pingcap:master Aug 31, 2022
@ti-chi-bot ti-chi-bot mentioned this pull request Aug 31, 2022
Closed
12 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #5753.

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Aug 31, 2022
@JaySon-Huang @ti-chi-bot
This is an automated cherry-pick of pingcap#5739
d85fa86 
Signed-off-by: ti-chi-bot <[email protected]>
@ti-chi-bot ti-chi-bot mentioned this pull request Aug 31, 2022
Closed
12 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #5754.

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Aug 31, 2022
@JaySon-Huang @ti-chi-bot
This is an automated cherry-pick of pingcap#5739
baac50c 
Signed-off-by: ti-chi-bot <[email protected]>
@JaySon-Huang JaySon-Huang deleted the fix_ps_gc_ext_time branch August 31, 2022 14:15
@sre-bot
Copy link
Collaborator

sre-bot commented Sep 1, 2022

Coverage for changed files

Filename                                             Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DeltaMerge/StoragePool.cpp                               268               117    56.34%          31                 9    70.97%         500               189    62.20%         138                71    48.55%
Page/V3/PageDirectory.cpp                                607                63    89.62%          31                 0   100.00%        1160               138    88.10%         444                79    82.21%
Page/V3/PageDirectory.h                                   27                 3    88.89%          27                 3    88.89%         110                10    90.91%           0                 0         -
Page/V3/PageDirectory/ExternalIdsByNamespace.cpp          14                 0   100.00%           4                 0   100.00%          39                 0   100.00%           8                 0   100.00%
Page/V3/PageDirectory/ExternalIdsByNamespace.h             1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Page/V3/PageDirectoryFactory.cpp                          55                 1    98.18%           6                 0   100.00%         127                 3    97.64%          50                 2    96.00%
Page/V3/PageStorageImpl.cpp                              138                24    82.61%          30                 4    86.67%         339                46    86.43%          70                15    78.57%
Page/V3/PageStorageImpl.h                                  9                 2    77.78%           9                 2    77.78%          23                 2    91.30%           0                 0         -
Page/V3/tests/gtest_page_directory.cpp                  1305               274    79.00%          65                 0   100.00%        2143                 7    99.67%         248               159    35.89%
Page/V3/tests/gtest_page_storage.cpp                     593               239    59.70%          51                 7    86.27%        1317               154    88.31%         216               118    45.37%
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                   3017               723    76.04%         255                25    90.20%        5759               549    90.47%        1174               444    62.18%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18488      8320             55.00%    213849  85844        59.86%

full coverage report (for internal network access only)

@JaySon-Huang JaySon-Huang mentioned this pull request Sep 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidezhu lidezhu lidezhu approved these changes

@flowbehappy flowbehappy flowbehappy approved these changes

Assignees
No one assigned
Labels
needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. needs-cherry-pick-release-6.2 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
Milestone
No milestone
Development

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