From 943b0e9113be4942872426227af2e33b7741e993 Mon Sep 17 00:00:00 2001 From: hehechen Date: Thu, 16 Jun 2022 15:11:08 +0800 Subject: [PATCH 1/5] optimize ps v3 restore Signed-off-by: hehechen --- dbms/src/Storages/Page/V3/PageDirectory.cpp | 25 +++++++++++++------ dbms/src/Storages/Page/V3/PageDirectory.h | 8 +++--- .../Storages/Page/V3/PageDirectoryFactory.cpp | 2 +- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index 5eb275f5af5..be1e8786018 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -479,7 +479,8 @@ bool VersionedPageEntries::cleanOutdatedEntries( UInt64 lowest_seq, std::map> * normal_entries_to_deref, PageEntriesV3 & entries_removed, - const PageLock & /*page_lock*/) + const PageLock & /*page_lock*/, + bool need_fill_entries_removed) { if (type == EditRecordType::VAR_EXTERNAL) { @@ -541,7 +542,10 @@ bool VersionedPageEntries::cleanOutdatedEntries( { if (iter->second.being_ref_count == 1) { - entries_removed.emplace_back(iter->second.entry); + if (need_fill_entries_removed) + { + entries_removed.emplace_back(iter->second.entry); + } iter = entries.erase(iter); } // The `being_ref_count` for this version is valid. While for older versions, @@ -551,7 +555,10 @@ bool VersionedPageEntries::cleanOutdatedEntries( else { // else there are newer "entry" in the version list, the outdated entries should be removed - entries_removed.emplace_back(iter->second.entry); + if (need_fill_entries_removed) + { + entries_removed.emplace_back(iter->second.entry); + } iter = entries.erase(iter); } } @@ -564,7 +571,7 @@ bool VersionedPageEntries::cleanOutdatedEntries( return entries.empty() || (entries.size() == 1 && entries.begin()->second.isDelete()); } -bool VersionedPageEntries::derefAndClean(UInt64 lowest_seq, PageIdV3Internal page_id, const PageVersion & deref_ver, const Int64 deref_count, PageEntriesV3 & entries_removed) +bool VersionedPageEntries::derefAndClean(UInt64 lowest_seq, PageIdV3Internal page_id, const PageVersion & deref_ver, const Int64 deref_count, PageEntriesV3 & entries_removed, bool need_fill_entries_removed) { auto page_lock = acquireLock(); if (type == EditRecordType::VAR_EXTERNAL) @@ -601,7 +608,7 @@ bool VersionedPageEntries::derefAndClean(UInt64 lowest_seq, PageIdV3Internal pag // Clean outdated entries after decreased the ref-counter // set `normal_entries_to_deref` to be nullptr to ignore cleaning ref-var-entries - return cleanOutdatedEntries(lowest_seq, /*normal_entries_to_deref*/ nullptr, entries_removed, page_lock); + return cleanOutdatedEntries(lowest_seq, /*normal_entries_to_deref*/ nullptr, entries_removed, page_lock, need_fill_entries_removed); } throw Exception(fmt::format("calling derefAndClean with invalid state [state={}]", toDebugString())); @@ -1239,7 +1246,7 @@ bool PageDirectory::tryDumpSnapshot(const ReadLimiterPtr & read_limiter, const W return done_any_io; } -PageEntriesV3 PageDirectory::gcInMemEntries() +PageEntriesV3 PageDirectory::gcInMemEntries(bool need_remove_blob) { UInt64 lowest_seq = sequence.load(); @@ -1304,7 +1311,8 @@ PageEntriesV3 PageDirectory::gcInMemEntries() lowest_seq, &normal_entries_to_deref, all_del_entries, - iter->second->acquireLock()); + iter->second->acquireLock(), + need_remove_blob); { std::unique_lock write_lock(table_rw_mutex); @@ -1342,7 +1350,8 @@ PageEntriesV3 PageDirectory::gcInMemEntries() page_id, /*deref_ver=*/deref_counter.first, /*deref_count=*/deref_counter.second, - all_del_entries); + all_del_entries, + need_remove_blob); if (all_deleted) { diff --git a/dbms/src/Storages/Page/V3/PageDirectory.h b/dbms/src/Storages/Page/V3/PageDirectory.h index bd7c433022f..81f11d90a68 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.h +++ b/dbms/src/Storages/Page/V3/PageDirectory.h @@ -224,13 +224,15 @@ class VersionedPageEntries UInt64 lowest_seq, std::map> * normal_entries_to_deref, PageEntriesV3 & entries_removed, - const PageLock & page_lock); + const PageLock & page_lock, + bool need_fill_entries_removed); bool derefAndClean( UInt64 lowest_seq, PageIdV3Internal page_id, const PageVersion & deref_ver, Int64 deref_count, - PageEntriesV3 & entries_removed); + PageEntriesV3 & entries_removed, + bool need_fill_entries_removed); void collapseTo(UInt64 seq, PageIdV3Internal page_id, PageEntriesEdit & edit); @@ -360,7 +362,7 @@ class PageDirectory bool tryDumpSnapshot(const ReadLimiterPtr & read_limiter = nullptr, const WriteLimiterPtr & write_limiter = nullptr); - PageEntriesV3 gcInMemEntries(); + PageEntriesV3 gcInMemEntries(bool need_remove_blob = true); std::set getAliveExternalIds(NamespaceId ns_id) const; diff --git a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp index 483c5073ab5..9e4c55a69bc 100644 --- a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp @@ -44,7 +44,7 @@ PageDirectoryPtr PageDirectoryFactory::createFromReader(String storage_name, WAL // After restoring from the disk, we need cleanup all invalid entries in memory, or it will // try to run GC again on some entries that are already marked as invalid in BlobStore. - dir->gcInMemEntries(); + dir->gcInMemEntries(false); LOG_FMT_INFO(DB::Logger::get("PageDirectoryFactory", storage_name), "PageDirectory restored [max_page_id={}] [max_applied_ver={}]", dir->getMaxId(), dir->sequence); if (blob_stats) From 5447f4f115caaeade9f5bfc20142b25225e74aec Mon Sep 17 00:00:00 2001 From: hehechen Date: Mon, 20 Jun 2022 14:25:45 +0800 Subject: [PATCH 2/5] Update dbms/src/Storages/Page/V3/PageDirectory.h Co-authored-by: JaySon --- dbms/src/Storages/Page/V3/PageDirectory.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.h b/dbms/src/Storages/Page/V3/PageDirectory.h index 81f11d90a68..03734ef2af6 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.h +++ b/dbms/src/Storages/Page/V3/PageDirectory.h @@ -362,7 +362,9 @@ class PageDirectory bool tryDumpSnapshot(const ReadLimiterPtr & read_limiter = nullptr, const WriteLimiterPtr & write_limiter = nullptr); - PageEntriesV3 gcInMemEntries(bool need_remove_blob = true); + // Perform a GC for in-memory entries and return the removed entries. + // If `return_removed_entries` is false, then just return an empty set. + PageEntriesV3 gcInMemEntries(bool return_removed_entries = true); std::set getAliveExternalIds(NamespaceId ns_id) const; From 2bac4eb08ed29074d0004c1eff3d0ec5662c0b73 Mon Sep 17 00:00:00 2001 From: hehechen Date: Mon, 20 Jun 2022 14:26:10 +0800 Subject: [PATCH 3/5] Update dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp Co-authored-by: JaySon --- dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp index 9e4c55a69bc..c25fdbfc838 100644 --- a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp @@ -44,7 +44,7 @@ PageDirectoryPtr PageDirectoryFactory::createFromReader(String storage_name, WAL // After restoring from the disk, we need cleanup all invalid entries in memory, or it will // try to run GC again on some entries that are already marked as invalid in BlobStore. - dir->gcInMemEntries(false); + dir->gcInMemEntries(/*return_removed_entries=*/false); LOG_FMT_INFO(DB::Logger::get("PageDirectoryFactory", storage_name), "PageDirectory restored [max_page_id={}] [max_applied_ver={}]", dir->getMaxId(), dir->sequence); if (blob_stats) From ca668324756b8ef57187041a1bdb15e6b0f0a39b Mon Sep 17 00:00:00 2001 From: hehechen Date: Mon, 20 Jun 2022 14:47:35 +0800 Subject: [PATCH 4/5] fix comments Signed-off-by: hehechen --- dbms/src/Storages/Page/V3/PageDirectory.cpp | 25 ++++++++----------- dbms/src/Storages/Page/V3/PageDirectory.h | 8 +++--- .../Storages/Page/V3/PageDirectoryFactory.cpp | 4 ++- .../Page/V3/tests/gtest_page_directory.cpp | 4 +-- 4 files changed, 19 insertions(+), 22 deletions(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index be1e8786018..37e3b1af40e 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -478,9 +478,8 @@ PageSize VersionedPageEntries::getEntriesByBlobIds( bool VersionedPageEntries::cleanOutdatedEntries( UInt64 lowest_seq, std::map> * normal_entries_to_deref, - PageEntriesV3 & entries_removed, - const PageLock & /*page_lock*/, - bool need_fill_entries_removed) + PageEntriesV3 * entries_removed, + const PageLock & /*page_lock*/) { if (type == EditRecordType::VAR_EXTERNAL) { @@ -542,9 +541,9 @@ bool VersionedPageEntries::cleanOutdatedEntries( { if (iter->second.being_ref_count == 1) { - if (need_fill_entries_removed) + if (entries_removed) { - entries_removed.emplace_back(iter->second.entry); + entries_removed->emplace_back(iter->second.entry); } iter = entries.erase(iter); } @@ -555,9 +554,9 @@ bool VersionedPageEntries::cleanOutdatedEntries( else { // else there are newer "entry" in the version list, the outdated entries should be removed - if (need_fill_entries_removed) + if (entries_removed) { - entries_removed.emplace_back(iter->second.entry); + entries_removed->emplace_back(iter->second.entry); } iter = entries.erase(iter); } @@ -571,7 +570,7 @@ bool VersionedPageEntries::cleanOutdatedEntries( return entries.empty() || (entries.size() == 1 && entries.begin()->second.isDelete()); } -bool VersionedPageEntries::derefAndClean(UInt64 lowest_seq, PageIdV3Internal page_id, const PageVersion & deref_ver, const Int64 deref_count, PageEntriesV3 & entries_removed, bool need_fill_entries_removed) +bool VersionedPageEntries::derefAndClean(UInt64 lowest_seq, PageIdV3Internal page_id, const PageVersion & deref_ver, const Int64 deref_count, PageEntriesV3 * entries_removed) { auto page_lock = acquireLock(); if (type == EditRecordType::VAR_EXTERNAL) @@ -608,7 +607,7 @@ bool VersionedPageEntries::derefAndClean(UInt64 lowest_seq, PageIdV3Internal pag // Clean outdated entries after decreased the ref-counter // set `normal_entries_to_deref` to be nullptr to ignore cleaning ref-var-entries - return cleanOutdatedEntries(lowest_seq, /*normal_entries_to_deref*/ nullptr, entries_removed, page_lock, need_fill_entries_removed); + return cleanOutdatedEntries(lowest_seq, /*normal_entries_to_deref*/ nullptr, entries_removed, page_lock); } throw Exception(fmt::format("calling derefAndClean with invalid state [state={}]", toDebugString())); @@ -1310,9 +1309,8 @@ PageEntriesV3 PageDirectory::gcInMemEntries(bool need_remove_blob) const bool all_deleted = iter->second->cleanOutdatedEntries( lowest_seq, &normal_entries_to_deref, - all_del_entries, - iter->second->acquireLock(), - need_remove_blob); + need_remove_blob ? &all_del_entries : nullptr, + iter->second->acquireLock()); { std::unique_lock write_lock(table_rw_mutex); @@ -1350,8 +1348,7 @@ PageEntriesV3 PageDirectory::gcInMemEntries(bool need_remove_blob) page_id, /*deref_ver=*/deref_counter.first, /*deref_count=*/deref_counter.second, - all_del_entries, - need_remove_blob); + need_remove_blob ? &all_del_entries : nullptr); if (all_deleted) { diff --git a/dbms/src/Storages/Page/V3/PageDirectory.h b/dbms/src/Storages/Page/V3/PageDirectory.h index 03734ef2af6..2f0f09f4e42 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.h +++ b/dbms/src/Storages/Page/V3/PageDirectory.h @@ -223,16 +223,14 @@ class VersionedPageEntries bool cleanOutdatedEntries( UInt64 lowest_seq, std::map> * normal_entries_to_deref, - PageEntriesV3 & entries_removed, - const PageLock & page_lock, - bool need_fill_entries_removed); + PageEntriesV3 * entries_removed, + const PageLock & page_lock); bool derefAndClean( UInt64 lowest_seq, PageIdV3Internal page_id, const PageVersion & deref_ver, Int64 deref_count, - PageEntriesV3 & entries_removed, - bool need_fill_entries_removed); + PageEntriesV3 * entries_removed); void collapseTo(UInt64 seq, PageIdV3Internal page_id, PageEntriesEdit & edit); diff --git a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp index c25fdbfc838..968049a3273 100644 --- a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp @@ -44,6 +44,7 @@ PageDirectoryPtr PageDirectoryFactory::createFromReader(String storage_name, WAL // After restoring from the disk, we need cleanup all invalid entries in memory, or it will // try to run GC again on some entries that are already marked as invalid in BlobStore. + // It's no need to remove the expired entries in BlobStore, so skip filling removed_entries to imporve performance. dir->gcInMemEntries(/*return_removed_entries=*/false); LOG_FMT_INFO(DB::Logger::get("PageDirectoryFactory", storage_name), "PageDirectory restored [max_page_id={}] [max_applied_ver={}]", dir->getMaxId(), dir->sequence); @@ -84,7 +85,8 @@ PageDirectoryPtr PageDirectoryFactory::createFromEdit(String storage_name, FileP // After restoring from the disk, we need cleanup all invalid entries in memory, or it will // try to run GC again on some entries that are already marked as invalid in BlobStore. - dir->gcInMemEntries(); + // It's no need to remove the expired entries in BlobStore when restore, so no need to fill removed_entries. + dir->gcInMemEntries(/*return_removed_entries=*/false); if (blob_stats) { diff --git a/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp b/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp index 83e07f75d37..6d6ef41630f 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp @@ -644,14 +644,14 @@ class VersionedEntriesTest : public ::testing::Test { DerefCounter deref_counter; PageEntriesV3 removed_entries; - bool all_removed = entries.cleanOutdatedEntries(seq, &deref_counter, removed_entries, entries.acquireLock()); + bool all_removed = entries.cleanOutdatedEntries(seq, &deref_counter, &removed_entries, entries.acquireLock()); return {all_removed, removed_entries, deref_counter}; } std::tuple runDeref(UInt64 seq, PageVersion ver, Int64 decrease_num) { PageEntriesV3 removed_entries; - bool all_removed = entries.derefAndClean(seq, buildV3Id(TEST_NAMESPACE_ID, page_id), ver, decrease_num, removed_entries); + bool all_removed = entries.derefAndClean(seq, buildV3Id(TEST_NAMESPACE_ID, page_id), ver, decrease_num, &removed_entries); return {all_removed, removed_entries}; } From 396420c058d6991e3f2fb0fbc10c88b5fdd4dc02 Mon Sep 17 00:00:00 2001 From: hehechen Date: Mon, 20 Jun 2022 16:34:17 +0800 Subject: [PATCH 5/5] fix Signed-off-by: hehechen --- dbms/src/Storages/Page/V3/PageDirectory.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index 37e3b1af40e..951da42de1c 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -1245,7 +1245,7 @@ bool PageDirectory::tryDumpSnapshot(const ReadLimiterPtr & read_limiter, const W return done_any_io; } -PageEntriesV3 PageDirectory::gcInMemEntries(bool need_remove_blob) +PageEntriesV3 PageDirectory::gcInMemEntries(bool return_removed_entries) { UInt64 lowest_seq = sequence.load(); @@ -1309,7 +1309,7 @@ PageEntriesV3 PageDirectory::gcInMemEntries(bool need_remove_blob) const bool all_deleted = iter->second->cleanOutdatedEntries( lowest_seq, &normal_entries_to_deref, - need_remove_blob ? &all_del_entries : nullptr, + return_removed_entries ? &all_del_entries : nullptr, iter->second->acquireLock()); { @@ -1348,7 +1348,7 @@ PageEntriesV3 PageDirectory::gcInMemEntries(bool need_remove_blob) page_id, /*deref_ver=*/deref_counter.first, /*deref_count=*/deref_counter.second, - need_remove_blob ? &all_del_entries : nullptr); + return_removed_entries ? &all_del_entries : nullptr); if (all_deleted) {