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: Fine grained lock on external callbacks #5699

Merged
merged 14 commits into from
Sep 5, 2022
Merged

PageStorage: Fine grained lock on external callbacks #5699

merged 14 commits into from
Sep 5, 2022

Conversation

JaySon-Huang
Copy link
Contributor

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

What problem does this PR solve?

Issue Number: close #5697

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

In PageStorageImpl::gcImpl clean_external_page, we hold the callbacks_mutex until all external page gc done.

tiflash/dbms/src/Storages/Page/V3/PageStorageImpl.cpp

Lines 287 to 298 in 7acf7f5

auto clean_external_page = [this]() {
std::scoped_lock lock{callbacks_mutex};
if (!callbacks_container.empty())
{
for (const auto & [ns_id, callbacks] : callbacks_container)
{
auto pending_external_pages = callbacks.scanner();
auto alive_external_ids = getAliveExternalPageIds(ns_id);
callbacks.remover(pending_external_pages, alive_external_ids);
}
}
};

while the time cost in clean_external_page is not ignorable and may block new StorageDeltaMerge from being created

tiflash/dbms/src/Storages/Page/V3/PageStorageImpl.cpp

Lines 400 to 408 in 7acf7f5

void PageStorageImpl::registerExternalPagesCallbacks(const ExternalPageCallbacks & callbacks)
{
std::scoped_lock lock{callbacks_mutex};
assert(callbacks.scanner != nullptr);
assert(callbacks.remover != nullptr);
assert(callbacks.ns_id != MAX_NAMESPACE_ID);
assert(callbacks_container.count(callbacks.ns_id) == 0);
callbacks_container.emplace(callbacks.ns_id, callbacks);
}

tiflash/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp

Lines 342 to 343 in 7acf7f5

// remember to unregister it when shutdown
storage_pool->dataRegisterExternalPagesCallbacks(callbacks);

What is changed and how it works?

  • Make the external pages cleaning callbacks safe even after table is dropped
  • Use shared_ptr to refine the lock granularity of running external cleaning callbacks, so that creating table won't be block by the whole processing time of cleanExternalPage.
  • Remove the non-atomic renaming table logic

Problem not yet resolve in this PR

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

Fix the bug that page GC may block creating tables

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 25, 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 release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-triage-completed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 25, 2022
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang
Copy link
Contributor Author

/run-unit-test

@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

1 similar comment
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@sre-bot

This comment was marked as outdated.

Sign in to view
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@sre-bot

This comment was marked as outdated.

Sign in to view
lidezhu
lidezhu previously approved these changes Aug 26, 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 with minor comment

dbms/src/Storages/Page/V3/tests/gtest_page_storage.cpp Outdated Show resolved Hide resolved
dbms/src/Storages/Page/V3/tests/gtest_page_storage.cpp Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 26, 2022
@JaySon-Huang JaySon-Huang changed the title PageStorage: Fine grained lock on external callbacks [WIP] PageStorage: Fine grained lock on external callbacks Aug 29, 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 Aug 29, 2022
@JaySon-Huang JaySon-Huang force-pushed the fix_ps_gctime branch 2 times, most recently from 71d36e1 to 8b3461d Compare August 29, 2022 08:04
@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 29, 2022
@JaySon-Huang
Remove useless clean_rename param
ac98793 
Signed-off-by: JaySon-Huang <tshent@qq.com>
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Sep 2, 2022

Coverage for changed files

Filename                                                              Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Databases/test/gtest_database.cpp                                         254                79    68.90%          19                 0   100.00%         726                10    98.62%          58                45    22.41%
Storages/DeltaMerge/DeltaMergeStore.cpp                                  1718               624    63.68%          67                 5    92.54%        2210               549    75.16%         764               323    57.72%
Storages/DeltaMerge/DeltaMergeStore.h                                      41                11    73.17%          19                 3    84.21%         107                27    74.77%          42                 8    80.95%
Storages/DeltaMerge/File/DMFile.cpp                                       365               131    64.11%          46                 2    95.65%         694               123    82.28%         202                62    69.31%
Storages/DeltaMerge/StoragePool.cpp                                       268               117    56.34%          31                 9    70.97%         500               189    62.20%         138                71    48.55%
Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp                  908               335    63.11%          68                 1    98.53%        2823                69    97.56%         264               183    30.68%
Storages/DeltaMerge/tests/gtest_dm_delta_merge_store_test_basic.h          22                 2    90.91%           6                 0   100.00%          81                 0   100.00%          18                 3    83.33%
Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp                198                43    78.28%          18                 0   100.00%         684                12    98.25%          56                28    50.00%
Storages/Page/PageStorage.h                                                22                 5    77.27%          22                 5    77.27%          97                13    86.60%           0                 0         -
Storages/Page/V3/PageDirectory/ExternalIdsByNamespace.h                     1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Storages/Page/V3/PageStorageImpl.cpp                                      155                24    84.52%          30                 4    86.67%         358                39    89.11%          74                15    79.73%
Storages/Page/V3/PageStorageImpl.h                                          9                 2    77.78%           9                 2    77.78%          23                 2    91.30%           0                 0         -
Storages/Page/V3/tests/gtest_page_storage.cpp                             639               259    59.47%          61                 7    88.52%        1439               162    88.74%         236               132    44.07%
Storages/PathPool.cpp                                                     469               197    58.00%          82                21    74.39%         773               213    72.45%         198                89    55.05%
Storages/PathPool.h                                                        14                 2    85.71%          14                 2    85.71%          16                 2    87.50%           0                 0         -
Storages/StorageDeltaMerge.cpp                                            711               335    52.88%          59                27    54.24%        1300               732    43.69%         364               236    35.16%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                    5794              2166    62.62%         552                88    84.06%       11832              2142    81.90%        2414              1195    50.50%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18641      8331             55.31%    215633  85923        60.15%

full coverage report (for internal network access only)

@lidezhu lidezhu removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 5, 2022
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 5, 2022
lidezhu
lidezhu approved these changes Sep 5, 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

@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Sep 5, 2022

Coverage for changed files

Filename                                                              Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Databases/test/gtest_database.cpp                                         254                79    68.90%          19                 0   100.00%         726                10    98.62%          58                45    22.41%
Storages/DeltaMerge/DeltaMergeStore.cpp                                  1724               624    63.81%          67                 5    92.54%        2225               549    75.33%         764               324    57.59%
Storages/DeltaMerge/DeltaMergeStore.h                                      41                12    70.73%          19                 3    84.21%         107                29    72.90%          42                 9    78.57%
Storages/DeltaMerge/File/DMFile.cpp                                       365               131    64.11%          46                 2    95.65%         694               123    82.28%         202                61    69.80%
Storages/DeltaMerge/StoragePool.cpp                                       268               117    56.34%          31                 9    70.97%         500               189    62.20%         138                71    48.55%
Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp                  908               335    63.11%          68                 1    98.53%        2823                69    97.56%         264               183    30.68%
Storages/DeltaMerge/tests/gtest_dm_delta_merge_store_test_basic.h          22                 2    90.91%           6                 0   100.00%          81                 0   100.00%          18                 3    83.33%
Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp                198                43    78.28%          18                 0   100.00%         684                12    98.25%          56                28    50.00%
Storages/Page/PageStorage.h                                                22                 5    77.27%          22                 5    77.27%          97                13    86.60%           0                 0         -
Storages/Page/V3/PageDirectory/ExternalIdsByNamespace.h                     1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Storages/Page/V3/PageStorageImpl.cpp                                      155                24    84.52%          30                 4    86.67%         358                39    89.11%          74                15    79.73%
Storages/Page/V3/PageStorageImpl.h                                          9                 2    77.78%           9                 2    77.78%          23                 2    91.30%           0                 0         -
Storages/Page/V3/tests/gtest_page_storage.cpp                             639               259    59.47%          61                 7    88.52%        1439               162    88.74%         236               132    44.07%
Storages/PathPool.cpp                                                     469               197    58.00%          82                21    74.39%         773               213    72.45%         198                89    55.05%
Storages/PathPool.h                                                        14                 2    85.71%          14                 2    85.71%          16                 2    87.50%           0                 0         -
Storages/StorageDeltaMerge.cpp                                            711               335    52.88%          59                27    54.24%        1300               732    43.69%         364               236    35.16%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                    5800              2167    62.64%         552                88    84.06%       11847              2144    81.90%        2414              1196    50.46%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18643      8333             55.30%    215651  85862        60.18%

full coverage report (for internal network access only)

@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 Sep 5, 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: 7091e1a

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

@JaySon-Huang: 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 Sep 5, 2022

Coverage for changed files

Filename                                                              Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Databases/test/gtest_database.cpp                                         254                79    68.90%          19                 0   100.00%         726                10    98.62%          58                45    22.41%
Storages/DeltaMerge/DeltaMergeStore.cpp                                  1724               624    63.81%          67                 5    92.54%        2225               549    75.33%         764               323    57.72%
Storages/DeltaMerge/DeltaMergeStore.h                                      41                11    73.17%          19                 3    84.21%         107                27    74.77%          42                 8    80.95%
Storages/DeltaMerge/File/DMFile.cpp                                       365               131    64.11%          46                 2    95.65%         694               123    82.28%         202                61    69.80%
Storages/DeltaMerge/StoragePool.cpp                                       268               117    56.34%          31                 9    70.97%         500               189    62.20%         138                71    48.55%
Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp                  908               335    63.11%          68                 1    98.53%        2823                69    97.56%         264               183    30.68%
Storages/DeltaMerge/tests/gtest_dm_delta_merge_store_test_basic.h          22                 2    90.91%           6                 0   100.00%          81                 0   100.00%          18                 3    83.33%
Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp                198                43    78.28%          18                 0   100.00%         684                12    98.25%          56                28    50.00%
Storages/Page/PageStorage.h                                                22                 5    77.27%          22                 5    77.27%          97                13    86.60%           0                 0         -
Storages/Page/V3/PageDirectory/ExternalIdsByNamespace.h                     1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Storages/Page/V3/PageStorageImpl.cpp                                      155                24    84.52%          30                 4    86.67%         358                39    89.11%          74                15    79.73%
Storages/Page/V3/PageStorageImpl.h                                          9                 2    77.78%           9                 2    77.78%          23                 2    91.30%           0                 0         -
Storages/Page/V3/tests/gtest_page_storage.cpp                             639               259    59.47%          61                 7    88.52%        1439               162    88.74%         236               132    44.07%
Storages/PathPool.cpp                                                     469               197    58.00%          82                21    74.39%         773               213    72.45%         198                89    55.05%
Storages/PathPool.h                                                        14                 2    85.71%          14                 2    85.71%          16                 2    87.50%           0                 0         -
Storages/StorageDeltaMerge.cpp                                            711               335    52.88%          59                27    54.24%        1300               732    43.69%         364               236    35.16%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                    5800              2166    62.66%         552                88    84.06%       11847              2142    81.92%        2414              1194    50.54%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18645      8333             55.31%    215664  85955        60.14%

full coverage report (for internal network access only)

@ti-chi-bot ti-chi-bot merged commit 4752b4b into pingcap:master Sep 5, 2022
@ti-chi-bot
Copy link
Member

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

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Sep 5, 2022
@JaySon-Huang @ti-chi-bot
This is an automated cherry-pick of pingcap#5699
7e2a091 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
This was referenced Sep 5, 2022
Closed
Closed
@ti-chi-bot
Copy link
Member

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

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Sep 5, 2022
@JaySon-Huang @ti-chi-bot
This is an automated cherry-pick of pingcap#5699
14b54d2 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@JaySon-Huang JaySon-Huang deleted the fix_ps_gctime branch September 5, 2022 06:49
@JaySon-Huang JaySon-Huang mentioned this pull request Dec 28, 2022
Merged
12 tasks
@ti-chi-bot ti-chi-bot mentioned this pull request Jan 9, 2023
Merged
12 tasks
@JaySon-Huang JaySon-Huang mentioned this pull request Feb 22, 2023
Merged
12 tasks
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

@lidezhu lidezhu lidezhu approved these changes

@breezewish breezewish Awaiting requested review from breezewish

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 Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ 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.

PageStorage gc for external pages my block creating tables
5 participants
@JaySon-Huang @ti-chi-bot @sre-bot @flowbehappy @lidezhu