From 2a13773a374d3a763b6fcaf6edf192a65d48edf7 Mon Sep 17 00:00:00 2001 From: Michael Xu Date: Thu, 22 Jun 2023 22:50:08 +0800 Subject: [PATCH 01/10] feat: Add support to PageGuard upgrade --- src/include/storage/page/page_guard.h | 35 ++++++++++++++++++++++++ src/storage/page/page_guard.cpp | 18 +++++++++++++ test/storage/page_guard_test.cpp | 38 ++++++++++++++++++++++++++- 3 files changed, 90 insertions(+), 1 deletion(-) diff --git a/src/include/storage/page/page_guard.h b/src/include/storage/page/page_guard.h index 1dcb2c60d..25ed2d238 100644 --- a/src/include/storage/page/page_guard.h +++ b/src/include/storage/page/page_guard.h @@ -5,6 +5,9 @@ namespace bustub { class BufferPoolManager; +// Forward declaration for LockUpgrade in BasicPageGuard +class ReadPageGuard; +class WritePageGuard; class BasicPageGuard { public: @@ -15,6 +18,26 @@ class BasicPageGuard { BasicPageGuard(const BasicPageGuard &) = delete; auto operator=(const BasicPageGuard &) -> BasicPageGuard & = delete; + /** + * @brief Provide guard upgrade functionality, from BasicPageGuard to ReadPageGuard + * You can call this function by: + * BasicPageGuard g; + * ReadPageGuard rg = g.UpgradeRead(); + * Note: You must not use the original BasicPageGuard after calling this function. + * @return ReadPageGuard + */ + auto UpgradeRead() -> ReadPageGuard; + + /** + * @brief Provide guard upgrade functionality, from BasicPageGuard to WritePageGuard + * You can call this function by: + * BasicPageGuard g; + * WritePageGuard rg = g.UpgradeWrite(); + * Note: You must not use the original BasicPageGuard after calling this function. + * @return WritePageGuard + */ + auto UpgradeWrite() -> WritePageGuard; + /** TODO(P1): Add implementation * * @brief Move constructor for BasicPageGuard @@ -78,6 +101,10 @@ class BasicPageGuard { return reinterpret_cast(GetDataMut()); } + // Used for GuardUpgradeTest + auto GetPage() -> const Page * { return page_; } + auto GetBPM() -> const BufferPoolManager * { return bpm_; } + private: friend class ReadPageGuard; friend class WritePageGuard; @@ -142,6 +169,10 @@ class ReadPageGuard { return guard_.As(); } + // Used for GuardUpgradeTest + auto GetPage() -> const Page * { return guard_.GetPage(); } + auto GetBPM() -> const BufferPoolManager * { return guard_.GetBPM(); } + private: // You may choose to get rid of this and add your own private variables. BasicPageGuard guard_; @@ -209,6 +240,10 @@ class WritePageGuard { return guard_.AsMut(); } + // Used for GuardUpgradeTest + auto GetPage() -> const Page * { return guard_.GetPage(); } + auto GetBPM() -> const BufferPoolManager * { return guard_.GetBPM(); } + private: // You may choose to get rid of this and add your own private variables. BasicPageGuard guard_; diff --git a/src/storage/page/page_guard.cpp b/src/storage/page/page_guard.cpp index 343c210c4..d94ece43b 100644 --- a/src/storage/page/page_guard.cpp +++ b/src/storage/page/page_guard.cpp @@ -3,6 +3,24 @@ namespace bustub { +auto BasicPageGuard::UpgradeRead() -> ReadPageGuard { + ReadPageGuard rpg(bpm_, page_); + // Set the two pointers to nullptr + bpm_ = nullptr; + page_ = nullptr; + // Return the rvalue, will invoke move assignment operator + return rpg; +} + +auto BasicPageGuard::UpgradeWrite() -> WritePageGuard { + WritePageGuard wpg(bpm_, page_); + // Set the two pointers to nullptr + bpm_ = nullptr; + page_ = nullptr; + // Return the rvalue + return wpg; +} + BasicPageGuard::BasicPageGuard(BasicPageGuard &&that) noexcept {} void BasicPageGuard::Drop() {} diff --git a/test/storage/page_guard_test.cpp b/test/storage/page_guard_test.cpp index 0c305cc06..0a773d97b 100644 --- a/test/storage/page_guard_test.cpp +++ b/test/storage/page_guard_test.cpp @@ -22,9 +22,45 @@ namespace bustub { +// NOLINTNEXTLINE +TEST(PageGuardTest, DISABLED_GuardUpgradeTest) { + auto disk_manager = std::make_shared(); + const size_t buffer_pool_size = 5; + const size_t k = 2; + auto *bpm = new BufferPoolManager(buffer_pool_size, disk_manager.get(), k); + + // Create two dummy pages + Page *dummy_page_1 = new Page(); + Page *dummy_page_2 = new Page(); + // Create two dummy BasicPageGuards + auto b_pg_1 = BasicPageGuard(bpm, dummy_page_1); + auto b_pg_2 = BasicPageGuard(bpm, dummy_page_2); + + // Upgrade the first pg to ReadPageGuard + ReadPageGuard rpg = b_pg_1.UpgradeRead(); + + // Sanity Check + assert(b_pg_1.GetPage() == nullptr && b_pg_1.GetBPM() == nullptr); + assert(rpg.GetPage() == dummy_page_1 && rpg.GetBPM() == bpm); + + // Upgrade the second pg to WritePageGuard + WritePageGuard wpg = b_pg_2.UpgradeWrite(); + + // Sanity Check + assert(b_pg_2.GetPage() == nullptr && b_pg_2.GetBPM() == nullptr); + assert(wpg.GetPage() == dummy_page_2 && wpg.GetBPM() == bpm); + + // Clean the resource + delete dummy_page_2; + delete dummy_page_1; + delete bpm; + + // Shut down the DiskManager + disk_manager->ShutDown(); +} + // NOLINTNEXTLINE TEST(PageGuardTest, DISABLED_SampleTest) { - const std::string db_name = "test.db"; const size_t buffer_pool_size = 5; const size_t k = 2; From 1677e23c63954faf3033e351de73851cc1b968f4 Mon Sep 17 00:00:00 2001 From: Michael Xu Date: Tue, 27 Jun 2023 13:19:37 +0800 Subject: [PATCH 02/10] feat: Update upgrade logic --- src/include/storage/page/page_guard.h | 2 +- src/storage/page/page_guard.cpp | 26 ++++++++++++++++---------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/include/storage/page/page_guard.h b/src/include/storage/page/page_guard.h index 25ed2d238..4bfc654e8 100644 --- a/src/include/storage/page/page_guard.h +++ b/src/include/storage/page/page_guard.h @@ -32,7 +32,7 @@ class BasicPageGuard { * @brief Provide guard upgrade functionality, from BasicPageGuard to WritePageGuard * You can call this function by: * BasicPageGuard g; - * WritePageGuard rg = g.UpgradeWrite(); + * WritePageGuard wg = g.UpgradeWrite(); * Note: You must not use the original BasicPageGuard after calling this function. * @return WritePageGuard */ diff --git a/src/storage/page/page_guard.cpp b/src/storage/page/page_guard.cpp index d94ece43b..51ca2d998 100644 --- a/src/storage/page/page_guard.cpp +++ b/src/storage/page/page_guard.cpp @@ -4,21 +4,27 @@ namespace bustub { auto BasicPageGuard::UpgradeRead() -> ReadPageGuard { - ReadPageGuard rpg(bpm_, page_); - // Set the two pointers to nullptr - bpm_ = nullptr; - page_ = nullptr; + // Save the current page id & bpm (This is because calling Drop() will set these to invalid) + auto *bpm = bpm_; + auto cur_pid = page_->GetPageId(); + // First drop the current page + this->Drop(); + // Fetch a new ReadPageGuard from the previously saved BPM + auto ret_pg = bpm->FetchPageRead(cur_pid); // Return the rvalue, will invoke move assignment operator - return rpg; + return ret_pg; } auto BasicPageGuard::UpgradeWrite() -> WritePageGuard { - WritePageGuard wpg(bpm_, page_); - // Set the two pointers to nullptr - bpm_ = nullptr; - page_ = nullptr; + // Save the current page id & bpm (Same as read) + auto *bpm = bpm_; + auto cur_pid = page_->GetPageId(); + // First drop the current page guard + this->Drop(); + // Fetch a new WritePageGuard from the previously saved BPM + auto ret_pg = bpm->FetchPageWrite(cur_pid); // Return the rvalue - return wpg; + return ret_pg; } BasicPageGuard::BasicPageGuard(BasicPageGuard &&that) noexcept {} From 8efd6cb3f9ffc0ee3b40295046ff8a0e2755b2e6 Mon Sep 17 00:00:00 2001 From: Zihao Xu Date: Sat, 1 Jul 2023 13:35:30 +0800 Subject: [PATCH 03/10] feat: Improve upgrade logic --- src/include/storage/page/page_guard.h | 23 +++++++++++-- src/storage/page/page_guard.cpp | 49 ++++++++++++++++++--------- 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/src/include/storage/page/page_guard.h b/src/include/storage/page/page_guard.h index 4bfc654e8..880e5f07f 100644 --- a/src/include/storage/page/page_guard.h +++ b/src/include/storage/page/page_guard.h @@ -18,22 +18,28 @@ class BasicPageGuard { BasicPageGuard(const BasicPageGuard &) = delete; auto operator=(const BasicPageGuard &) -> BasicPageGuard & = delete; - /** + /** TODO(P1): Add implementation + * * @brief Provide guard upgrade functionality, from BasicPageGuard to ReadPageGuard + * * You can call this function by: * BasicPageGuard g; * ReadPageGuard rg = g.UpgradeRead(); * Note: You must not use the original BasicPageGuard after calling this function. + * * @return ReadPageGuard */ auto UpgradeRead() -> ReadPageGuard; - /** + /** TODO(P1): Add implementation + * * @brief Provide guard upgrade functionality, from BasicPageGuard to WritePageGuard + * * You can call this function by: * BasicPageGuard g; * WritePageGuard wg = g.UpgradeWrite(); * Note: You must not use the original BasicPageGuard after calling this function. + * * @return WritePageGuard */ auto UpgradeWrite() -> WritePageGuard; @@ -121,6 +127,19 @@ class ReadPageGuard { ReadPageGuard(const ReadPageGuard &) = delete; auto operator=(const ReadPageGuard &) -> ReadPageGuard & = delete; + /** TODO(P1): Add implementation + * + * @brief Provide guard upgrade functionality, from ReadPageGuard to WritePageGuard + * + * You can call this function by: + * ReadPageGuard rg; + * WritePageGuard wg = rg.UpgradeWrite(); + * Note: You must not use the original ReadPageGuard after calling this function. + * + * @return WritePageGuard + */ + auto UpgradeWrite() -> WritePageGuard; + /** TODO(P1): Add implementation * * @brief Move constructor for ReadPageGuard diff --git a/src/storage/page/page_guard.cpp b/src/storage/page/page_guard.cpp index 51ca2d998..de987a64f 100644 --- a/src/storage/page/page_guard.cpp +++ b/src/storage/page/page_guard.cpp @@ -4,27 +4,44 @@ namespace bustub { auto BasicPageGuard::UpgradeRead() -> ReadPageGuard { - // Save the current page id & bpm (This is because calling Drop() will set these to invalid) - auto *bpm = bpm_; - auto cur_pid = page_->GetPageId(); - // First drop the current page - this->Drop(); - // Fetch a new ReadPageGuard from the previously saved BPM - auto ret_pg = bpm->FetchPageRead(cur_pid); + // Basic sanity check + BUSTUB_ENSURE((bpm_ != nullptr && page_ != nullptr), "The BPM & Page in the current guard must not be NULL"); + // Manually lock the underlying page with read lock + page_->RLatch(); + // Construct the newly returned `ReadPageGuard` + ReadPageGuard ret_rpg = ReadPageGuard{bpm_, page_}; + // Set the local variables to null + bpm_ = nullptr; + page_ = nullptr; // Return the rvalue, will invoke move assignment operator - return ret_pg; + return ret_rpg; } auto BasicPageGuard::UpgradeWrite() -> WritePageGuard { - // Save the current page id & bpm (Same as read) - auto *bpm = bpm_; - auto cur_pid = page_->GetPageId(); - // First drop the current page guard - this->Drop(); - // Fetch a new WritePageGuard from the previously saved BPM - auto ret_pg = bpm->FetchPageWrite(cur_pid); + // TODO(p1): Your WritePageGuard upgrade logic here +} + +auto ReadPageGuard::UpgradeWrite() -> WritePageGuard { + // Get the underlying page from guard_ + auto cur_page = guard_->GetPage(); + // Perform sanity check + BUSTUB_ENSURE((cur_page != nullptr), "The Page in the guard_ must not be NULL"); + // Release the current read lock (Since the pin count is not changed, the current page will not be evicted) + // But I think the semantic here may be further checked, since this operation is not actually `atomic` to user + // e.g, There may be other thread wanting to acquire write lock at the same time, + // also some read operations may happen concurrently, causing potential blocking + // P.S. The std::shared_mutex doesn't officially support atomic lock upgrade :( + // Boost Lib has some of the functionalities(atomic mutex upgrade), but it's not allowed here though + cur_page->RUnlock(); + // Acquire new write lock + cur_page->WLatch(); + // Construct a newly returned `WritePageGuard` + WritePageGuard ret_wpg = WritePageGuard{bpm_, page_}; + // Set the local variables to null + bpm_ = nullptr; + page_ = nullptr; // Return the rvalue - return ret_pg; + return ret_wpg; } BasicPageGuard::BasicPageGuard(BasicPageGuard &&that) noexcept {} From 6099836676cd0886b134a154026922e92061174a Mon Sep 17 00:00:00 2001 From: Zihao Xu Date: Sat, 1 Jul 2023 14:11:16 +0800 Subject: [PATCH 04/10] feat: Improve upgrade logic & Ensure format consistency --- src/include/storage/page/page_guard.h | 33 ++++++++++++++------ src/storage/page/page_guard.cpp | 13 ++++---- test/storage/b_plus_tree_concurrent_test.cpp | 2 +- 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/include/storage/page/page_guard.h b/src/include/storage/page/page_guard.h index 880e5f07f..4575a4ecf 100644 --- a/src/include/storage/page/page_guard.h +++ b/src/include/storage/page/page_guard.h @@ -107,9 +107,16 @@ class BasicPageGuard { return reinterpret_cast(GetDataMut()); } - // Used for GuardUpgradeTest - auto GetPage() -> const Page * { return page_; } - auto GetBPM() -> const BufferPoolManager * { return bpm_; } + /** Getter Methods */ + auto GetPage() const -> Page * { return page_; } + + auto GetBPM() const -> BufferPoolManager * { return bpm_; } + + /** Set the local variables to NULL */ + void SetNull() noexcept { + page_ = nullptr; + bpm_ = nullptr; + } private: friend class ReadPageGuard; @@ -188,9 +195,13 @@ class ReadPageGuard { return guard_.As(); } - // Used for GuardUpgradeTest - auto GetPage() -> const Page * { return guard_.GetPage(); } - auto GetBPM() -> const BufferPoolManager * { return guard_.GetBPM(); } + /** Getter Methods */ + auto GetPage() const -> Page * { return guard_.GetPage(); } + + auto GetBPM() const -> BufferPoolManager * { return guard_.GetBPM(); } + + /** Set the guard_'s local variables to NULL */ + void SetNull() noexcept { guard_.SetNull(); } private: // You may choose to get rid of this and add your own private variables. @@ -259,9 +270,13 @@ class WritePageGuard { return guard_.AsMut(); } - // Used for GuardUpgradeTest - auto GetPage() -> const Page * { return guard_.GetPage(); } - auto GetBPM() -> const BufferPoolManager * { return guard_.GetBPM(); } + /** Getter Methods */ + auto GetPage() const -> Page * { return guard_.GetPage(); } + + auto GetBPM() const -> BufferPoolManager * { return guard_.GetBPM(); } + + /** Set the guard_'s local variables to NULL */ + void SetNull() noexcept { guard_.SetNull(); } private: // You may choose to get rid of this and add your own private variables. diff --git a/src/storage/page/page_guard.cpp b/src/storage/page/page_guard.cpp index de987a64f..237e9a919 100644 --- a/src/storage/page/page_guard.cpp +++ b/src/storage/page/page_guard.cpp @@ -11,19 +11,19 @@ auto BasicPageGuard::UpgradeRead() -> ReadPageGuard { // Construct the newly returned `ReadPageGuard` ReadPageGuard ret_rpg = ReadPageGuard{bpm_, page_}; // Set the local variables to null - bpm_ = nullptr; - page_ = nullptr; + SetNull(); // Return the rvalue, will invoke move assignment operator return ret_rpg; } auto BasicPageGuard::UpgradeWrite() -> WritePageGuard { // TODO(p1): Your WritePageGuard upgrade logic here + return {nullptr, nullptr}; } auto ReadPageGuard::UpgradeWrite() -> WritePageGuard { // Get the underlying page from guard_ - auto cur_page = guard_->GetPage(); + Page *cur_page = guard_.GetPage(); // Perform sanity check BUSTUB_ENSURE((cur_page != nullptr), "The Page in the guard_ must not be NULL"); // Release the current read lock (Since the pin count is not changed, the current page will not be evicted) @@ -32,14 +32,13 @@ auto ReadPageGuard::UpgradeWrite() -> WritePageGuard { // also some read operations may happen concurrently, causing potential blocking // P.S. The std::shared_mutex doesn't officially support atomic lock upgrade :( // Boost Lib has some of the functionalities(atomic mutex upgrade), but it's not allowed here though - cur_page->RUnlock(); + cur_page->RUnlatch(); // Acquire new write lock cur_page->WLatch(); // Construct a newly returned `WritePageGuard` - WritePageGuard ret_wpg = WritePageGuard{bpm_, page_}; + WritePageGuard ret_wpg = WritePageGuard{guard_.GetBPM(), guard_.GetPage()}; // Set the local variables to null - bpm_ = nullptr; - page_ = nullptr; + guard_.SetNull(); // Return the rvalue return ret_wpg; } diff --git a/test/storage/b_plus_tree_concurrent_test.cpp b/test/storage/b_plus_tree_concurrent_test.cpp index 666c124a6..e7a4a1606 100644 --- a/test/storage/b_plus_tree_concurrent_test.cpp +++ b/test/storage/b_plus_tree_concurrent_test.cpp @@ -27,7 +27,7 @@ using bustub::DiskManagerUnlimitedMemory; // helper function to launch multiple threads template -void LaunchParallelTest(uint64_t num_threads, Args &&...args) { +void LaunchParallelTest(uint64_t num_threads, Args &&... args) { std::vector thread_group; // Launch a group of threads From a7d2a0d766ea4d05bd345e7f317f73acf98a00d3 Mon Sep 17 00:00:00 2001 From: Zihao Xu Date: Sat, 1 Jul 2023 14:24:47 +0800 Subject: [PATCH 05/10] feat: Ensure format consistency --- test/storage/b_plus_tree_concurrent_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/storage/b_plus_tree_concurrent_test.cpp b/test/storage/b_plus_tree_concurrent_test.cpp index e7a4a1606..666c124a6 100644 --- a/test/storage/b_plus_tree_concurrent_test.cpp +++ b/test/storage/b_plus_tree_concurrent_test.cpp @@ -27,7 +27,7 @@ using bustub::DiskManagerUnlimitedMemory; // helper function to launch multiple threads template -void LaunchParallelTest(uint64_t num_threads, Args &&... args) { +void LaunchParallelTest(uint64_t num_threads, Args &&...args) { std::vector thread_group; // Launch a group of threads From 8ed1decdad3b4b0e9ff2f87e676a00d93888c469 Mon Sep 17 00:00:00 2001 From: Zihao Xu Date: Sat, 1 Jul 2023 16:47:32 +0800 Subject: [PATCH 06/10] feat: Update test cases & Fix typos --- test/storage/b_plus_tree_concurrent_test.cpp | 14 +-- test/storage/page_guard_test.cpp | 117 ++++++++++++++++++- 2 files changed, 119 insertions(+), 12 deletions(-) diff --git a/test/storage/b_plus_tree_concurrent_test.cpp b/test/storage/b_plus_tree_concurrent_test.cpp index 666c124a6..daa372053 100644 --- a/test/storage/b_plus_tree_concurrent_test.cpp +++ b/test/storage/b_plus_tree_concurrent_test.cpp @@ -57,7 +57,7 @@ void InsertHelper(BPlusTree, RID, GenericComparator<8>> *tree, con delete transaction; } -// helper function to seperate insert +// helper function to separate insert void InsertHelperSplit(BPlusTree, RID, GenericComparator<8>> *tree, const std::vector &keys, int total_threads, __attribute__((unused)) uint64_t thread_itr) { GenericKey<8> index_key; @@ -88,7 +88,7 @@ void DeleteHelper(BPlusTree, RID, GenericComparator<8>> *tree, con delete transaction; } -// helper function to seperate delete +// helper function to separate delete void DeleteHelperSplit(BPlusTree, RID, GenericComparator<8>> *tree, const std::vector &remove_keys, int total_threads, __attribute__((unused)) uint64_t thread_itr) { @@ -353,24 +353,24 @@ TEST(BPlusTreeConcurrentTest, DISABLED_MixTest2) { BPlusTree, RID, GenericComparator<8>> tree("foo_pk", page_id, bpm, comparator); // Add perserved_keys - std::vector perserved_keys; + std::vector preserved_keys; std::vector dynamic_keys; int64_t total_keys = 50; int64_t sieve = 5; for (int64_t i = 1; i <= total_keys; i++) { if (i % sieve == 0) { - perserved_keys.push_back(i); + preserved_keys.push_back(i); } else { dynamic_keys.push_back(i); } } - InsertHelper(&tree, perserved_keys, 1); + InsertHelper(&tree, preserved_keys, 1); // Check there are 1000 keys in there size_t size; auto insert_task = [&](int tid) { InsertHelper(&tree, dynamic_keys, tid); }; auto delete_task = [&](int tid) { DeleteHelper(&tree, dynamic_keys, tid); }; - auto lookup_task = [&](int tid) { LookupHelper(&tree, perserved_keys, tid); }; + auto lookup_task = [&](int tid) { LookupHelper(&tree, preserved_keys, tid); }; std::vector threads; std::vector> tasks; @@ -396,7 +396,7 @@ TEST(BPlusTreeConcurrentTest, DISABLED_MixTest2) { } } - ASSERT_EQ(size, perserved_keys.size()); + ASSERT_EQ(size, preserved_keys.size()); bpm->UnpinPage(HEADER_PAGE_ID, true); delete bpm; diff --git a/test/storage/page_guard_test.cpp b/test/storage/page_guard_test.cpp index 0a773d97b..afe7c9f5f 100644 --- a/test/storage/page_guard_test.cpp +++ b/test/storage/page_guard_test.cpp @@ -11,8 +11,10 @@ //===----------------------------------------------------------------------===// #include +#include #include #include +#include #include "buffer/buffer_pool_manager.h" #include "storage/disk/disk_manager_memory.h" @@ -23,18 +25,20 @@ namespace bustub { // NOLINTNEXTLINE -TEST(PageGuardTest, DISABLED_GuardUpgradeTest) { - auto disk_manager = std::make_shared(); +TEST(PageGuardTest, DISABLED_GuardUpgradeBasicTest1) { const size_t buffer_pool_size = 5; const size_t k = 2; + + auto disk_manager = std::make_shared(); auto *bpm = new BufferPoolManager(buffer_pool_size, disk_manager.get(), k); // Create two dummy pages Page *dummy_page_1 = new Page(); Page *dummy_page_2 = new Page(); + // Create two dummy BasicPageGuards - auto b_pg_1 = BasicPageGuard(bpm, dummy_page_1); - auto b_pg_2 = BasicPageGuard(bpm, dummy_page_2); + auto b_pg_1 = BasicPageGuard{bpm, dummy_page_1}; + auto b_pg_2 = BasicPageGuard{bpm, dummy_page_2}; // Upgrade the first pg to ReadPageGuard ReadPageGuard rpg = b_pg_1.UpgradeRead(); @@ -59,6 +63,109 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeTest) { disk_manager->ShutDown(); } +// NOLINTNEXTLINE +TEST(PageGuardTest, DISABLED_GuardUpgradeBasicTest2) { + const size_t buffer_pool_size = 5; + const size_t k = 2; + + auto disk_manager = std::make_shared(); + auto *bpm = new BufferPoolManager(buffer_pool_size, disk_manager.get(), k); + + // Create one dummy pages + Page *dummy_page = new Page(); + + // Create one dummy BasicPageGuard + auto b_pg = BasicPageGuard{bpm, dummy_page}; + + // Upgrade the pg to ReadPageGuard + ReadPageGuard rpg = b_pg.UpgradeRead(); + + // Sanity Check + assert(b_pg.GetPage() == nullptr && b_pg.GetBPM() == nullptr); + assert(rpg.GetPage() == dummy_page && rpg.GetBPM() == bpm); + + // Upgrade the rpg to WritePageGuard + WritePageGuard wpg = rpg.UpgradeWrite(); + + // Sanity Check + assert(rpg.GetPage() == nullptr && rpg.GetBPM() == nullptr); + assert(wpg.GetPage() == dummy_page && wpg.GetBPM() == bpm); + + // Clean the resource + delete dummy_page; + delete bpm; + + // Shut down the DiskManager + disk_manager->ShutDown(); +} + +// NOLINTNEXTLINE +TEST(PageGuardTest, DISABLED_GuardUpgradeConcurrentTest1) { + // This test should not block + const size_t buffer_pool_size = 5; + const size_t k = 2; + + auto disk_manager = std::make_shared(); + auto *bpm = new BufferPoolManager(buffer_pool_size, disk_manager.get(), k); + + // Create ten dummy pages, construct a page group + std::vector> p_g; + + // Reserve space for p_g, since clang-tidy complains + p_g.reserve(10); + + // Initialize the page group + for (int i = 0; i < 10; ++i) { + p_g.push_back(std::make_shared()); + } + + // Create a bpg group + std::vector bpg_g; + + // Create a rpg group + std::vector> rpg_g; + + // Reserve space for bpg_g & rpg_g, same as above + bpg_g.reserve(10); + rpg_g.reserve(10); + + // Initialize bpg_g + for (int i = 0; i < 10; ++i) { + bpg_g.emplace_back(bpm, p_g[i].get()); + } + + // Create the thread vector + std::vector t_g; + + // Reserve space + t_g.reserve(10); + + // Launch ten threads, performing the upgrade + for (int i = 0; i < 10; ++i) { + // Capture i by value here, to avoid potential race condition + t_g.emplace_back([i, &bpg_g, &rpg_g]() { + rpg_g.emplace_back(bpg_g[i].UpgradeRead(), i); + }); + } + + // Wait til all the upgrades are finished + for (auto &t : t_g) { + t.join(); + } + + // Sanity check + for (const auto &[rpg, index] : rpg_g) { + assert(bpg_g[index].GetPage() == nullptr && bpg_g[index].GetBPM() == nullptr); + assert(rpg.GetPage() == p_g[index].get() && rpg.GetBPM() == bpm); + } + + // Clean the resource + delete bpm; + + // Shutdown the Disk Manager + disk_manager->ShutDown(); +} + // NOLINTNEXTLINE TEST(PageGuardTest, DISABLED_SampleTest) { const size_t buffer_pool_size = 5; @@ -70,7 +177,7 @@ TEST(PageGuardTest, DISABLED_SampleTest) { page_id_t page_id_temp; auto *page0 = bpm->NewPage(&page_id_temp); - auto guarded_page = BasicPageGuard(bpm.get(), page0); + auto guarded_page = BasicPageGuard{bpm.get(), page0}; EXPECT_EQ(page0->GetData(), guarded_page.GetData()); EXPECT_EQ(page0->GetPageId(), guarded_page.PageId()); From 0793dc77be14fabf549d86f3cfebea3971e48cb2 Mon Sep 17 00:00:00 2001 From: Zihao Xu Date: Sat, 1 Jul 2023 16:52:07 +0800 Subject: [PATCH 07/10] feat: Ensure format consistency --- test/storage/page_guard_test.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/storage/page_guard_test.cpp b/test/storage/page_guard_test.cpp index afe7c9f5f..8b72be0d4 100644 --- a/test/storage/page_guard_test.cpp +++ b/test/storage/page_guard_test.cpp @@ -14,7 +14,6 @@ #include #include #include -#include #include "buffer/buffer_pool_manager.h" #include "storage/disk/disk_manager_memory.h" @@ -143,9 +142,7 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeConcurrentTest1) { // Launch ten threads, performing the upgrade for (int i = 0; i < 10; ++i) { // Capture i by value here, to avoid potential race condition - t_g.emplace_back([i, &bpg_g, &rpg_g]() { - rpg_g.emplace_back(bpg_g[i].UpgradeRead(), i); - }); + t_g.emplace_back([i, &bpg_g, &rpg_g]() { rpg_g.emplace_back(bpg_g[i].UpgradeRead(), i); }); } // Wait til all the upgrades are finished From d7bf356ee1e7adccd71a238936d3e8c018da1d51 Mon Sep 17 00:00:00 2001 From: Zihao Xu Date: Mon, 24 Jul 2023 18:00:26 +0800 Subject: [PATCH 08/10] feat: improve test cases --- test/storage/page_guard_test.cpp | 47 ++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/test/storage/page_guard_test.cpp b/test/storage/page_guard_test.cpp index 8b72be0d4..41a81c94b 100644 --- a/test/storage/page_guard_test.cpp +++ b/test/storage/page_guard_test.cpp @@ -16,6 +16,7 @@ #include #include "buffer/buffer_pool_manager.h" +#include "common/config.h" #include "storage/disk/disk_manager_memory.h" #include "storage/page/page_guard.h" @@ -29,34 +30,36 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeBasicTest1) { const size_t k = 2; auto disk_manager = std::make_shared(); - auto *bpm = new BufferPoolManager(buffer_pool_size, disk_manager.get(), k); + auto bpm = std::make_shared(buffer_pool_size, disk_manager.get(), k); - // Create two dummy pages - Page *dummy_page_1 = new Page(); - Page *dummy_page_2 = new Page(); + // Create two dummy pages from BPM + page_id_t p_1 = -1; + page_id_t p_2 = -1; + Page *dummy_page_1 = bpm->NewPage(&p_1); + Page *dummy_page_2 = bpm->NewPage(&p_2); + EXPECT_EQ(dummy_page_1->GetPinCount(), 1); + EXPECT_EQ(dummy_page_2->GetPinCount(), 1); + assert(p_1 != -1 && p_2 != -1); + assert(dummy_page_1 && dummy_page_2); // Create two dummy BasicPageGuards - auto b_pg_1 = BasicPageGuard{bpm, dummy_page_1}; - auto b_pg_2 = BasicPageGuard{bpm, dummy_page_2}; + auto b_pg_1 = bpm->FetchPageBasic(p_1); + auto b_pg_2 = bpm->FetchPageBasic(p_2); - // Upgrade the first pg to ReadPageGuard + // Upgrade the first PageGuard to ReadPageGuard ReadPageGuard rpg = b_pg_1.UpgradeRead(); // Sanity Check - assert(b_pg_1.GetPage() == nullptr && b_pg_1.GetBPM() == nullptr); - assert(rpg.GetPage() == dummy_page_1 && rpg.GetBPM() == bpm); + EXPECT_EQ(b_pg_1.GetPage(), nullptr); + EXPECT_EQ(b_pg_1.GetBPM(), nullptr); + assert(rpg.GetPage() == dummy_page_1 && rpg.GetBPM() == bpm.get()); - // Upgrade the second pg to WritePageGuard + // Upgrade the second PageGuard to WritePageGuard WritePageGuard wpg = b_pg_2.UpgradeWrite(); // Sanity Check assert(b_pg_2.GetPage() == nullptr && b_pg_2.GetBPM() == nullptr); - assert(wpg.GetPage() == dummy_page_2 && wpg.GetBPM() == bpm); - - // Clean the resource - delete dummy_page_2; - delete dummy_page_1; - delete bpm; + assert(wpg.GetPage() == dummy_page_2 && wpg.GetBPM() == bpm.get()); // Shut down the DiskManager disk_manager->ShutDown(); @@ -76,7 +79,7 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeBasicTest2) { // Create one dummy BasicPageGuard auto b_pg = BasicPageGuard{bpm, dummy_page}; - // Upgrade the pg to ReadPageGuard + // Upgrade the PageGuard to ReadPageGuard ReadPageGuard rpg = b_pg.UpgradeRead(); // Sanity Check @@ -163,6 +166,16 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeConcurrentTest1) { disk_manager->ShutDown(); } +TEST(PageGuardTest, DISABLED_GuardUpgradeConcurrentTest2) { + const size_t buffer_pool_size = 5; + const size_t k = 2; + + auto disk_manager = std::make_shared(); + auto *bpm = new BufferPoolManager(buffer_pool_size, disk_manager.get(), k); + + +} + // NOLINTNEXTLINE TEST(PageGuardTest, DISABLED_SampleTest) { const size_t buffer_pool_size = 5; From 34b6f8e4f8e7effa63247bfaefadc59f45b74567 Mon Sep 17 00:00:00 2001 From: Michael Xu Date: Mon, 24 Jul 2023 21:56:53 +0800 Subject: [PATCH 09/10] feat: improve tests --- test/storage/page_guard_test.cpp | 141 ++++++++++++++++++++++++------- 1 file changed, 112 insertions(+), 29 deletions(-) diff --git a/test/storage/page_guard_test.cpp b/test/storage/page_guard_test.cpp index 41a81c94b..e9c7991da 100644 --- a/test/storage/page_guard_test.cpp +++ b/test/storage/page_guard_test.cpp @@ -37,10 +37,17 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeBasicTest1) { page_id_t p_2 = -1; Page *dummy_page_1 = bpm->NewPage(&p_1); Page *dummy_page_2 = bpm->NewPage(&p_2); + // Now the pin counts should both be 1 EXPECT_EQ(dummy_page_1->GetPinCount(), 1); EXPECT_EQ(dummy_page_2->GetPinCount(), 1); - assert(p_1 != -1 && p_2 != -1); - assert(dummy_page_1 && dummy_page_2); + EXPECT_NE(p_1, -1); + EXPECT_NE(p_2, -1); + EXPECT_NE(dummy_page_1, nullptr); + EXPECT_NE(dummy_page_2, nullptr); + + // Unpin the two pages + bpm->UnpinPage(p_1, false); + bpm->UnpinPage(p_2, false); // Create two dummy BasicPageGuards auto b_pg_1 = bpm->FetchPageBasic(p_1); @@ -50,16 +57,29 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeBasicTest1) { ReadPageGuard rpg = b_pg_1.UpgradeRead(); // Sanity Check + EXPECT_EQ(dummy_page_1->GetPinCount(), 1); EXPECT_EQ(b_pg_1.GetPage(), nullptr); EXPECT_EQ(b_pg_1.GetBPM(), nullptr); - assert(rpg.GetPage() == dummy_page_1 && rpg.GetBPM() == bpm.get()); + EXPECT_EQ(rpg.GetPage(), dummy_page_1); + EXPECT_EQ(rpg.GetBPM(), bpm.get()); + + // Drop the first ReadPageGuard + rpg.Drop(); + EXPECT_EQ(dummy_page_1->GetPinCount(), 0); // Upgrade the second PageGuard to WritePageGuard WritePageGuard wpg = b_pg_2.UpgradeWrite(); // Sanity Check - assert(b_pg_2.GetPage() == nullptr && b_pg_2.GetBPM() == nullptr); - assert(wpg.GetPage() == dummy_page_2 && wpg.GetBPM() == bpm.get()); + EXPECT_EQ(dummy_page_2->GetPinCount(), 1); + EXPECT_EQ(b_pg_2.GetPage(), nullptr); + EXPECT_EQ(b_pg_2.GetBPM(), nullptr); + EXPECT_EQ(wpg.GetPage(), dummy_page_2); + EXPECT_EQ(wpg.GetBPM(), bpm.get()); + + // Drop the second WritePageGuard + wpg.Drop(); + EXPECT_EQ(dummy_page_2->GetPinCount(), 0); // Shut down the DiskManager disk_manager->ShutDown(); @@ -71,31 +91,45 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeBasicTest2) { const size_t k = 2; auto disk_manager = std::make_shared(); - auto *bpm = new BufferPoolManager(buffer_pool_size, disk_manager.get(), k); + auto bpm = std::make_shared(buffer_pool_size, disk_manager.get(), k); // Create one dummy pages - Page *dummy_page = new Page(); + page_id_t p_id = -1; + Page *dummy_page = bpm->NewPage(&p_id); + // The pin count should be 1 + EXPECT_EQ(dummy_page->GetPinCount(), 1); + EXPECT_NE(p_id, -1); + EXPECT_NE(dummy_page, nullptr); + + // Unpin the page + bpm->UnpinPage(p_id, false); // Create one dummy BasicPageGuard - auto b_pg = BasicPageGuard{bpm, dummy_page}; + auto bpg = bpm->FetchPageBasic(p_id); // Upgrade the PageGuard to ReadPageGuard - ReadPageGuard rpg = b_pg.UpgradeRead(); + ReadPageGuard rpg = bpg.UpgradeRead(); // Sanity Check - assert(b_pg.GetPage() == nullptr && b_pg.GetBPM() == nullptr); - assert(rpg.GetPage() == dummy_page && rpg.GetBPM() == bpm); + EXPECT_EQ(dummy_page->GetPinCount(), 1); + EXPECT_EQ(bpg.GetPage(), nullptr); + EXPECT_EQ(bpg.GetBPM(), nullptr); + EXPECT_EQ(rpg.GetPage(), dummy_page); + EXPECT_EQ(rpg.GetBPM(), bpm.get()); // Upgrade the rpg to WritePageGuard WritePageGuard wpg = rpg.UpgradeWrite(); // Sanity Check - assert(rpg.GetPage() == nullptr && rpg.GetBPM() == nullptr); - assert(wpg.GetPage() == dummy_page && wpg.GetBPM() == bpm); + EXPECT_EQ(dummy_page->GetPinCount(), 1); + EXPECT_EQ(rpg.GetPage(), nullptr); + EXPECT_EQ(rpg.GetBPM(), nullptr); + EXPECT_EQ(wpg.GetPage(), dummy_page); + EXPECT_EQ(wpg.GetBPM(), bpm.get()); - // Clean the resource - delete dummy_page; - delete bpm; + // Drop the WritePageGuard + wpg.Drop(); + EXPECT_EQ(dummy_page->GetPinCount(), 0); // Shut down the DiskManager disk_manager->ShutDown(); @@ -104,21 +138,29 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeBasicTest2) { // NOLINTNEXTLINE TEST(PageGuardTest, DISABLED_GuardUpgradeConcurrentTest1) { // This test should not block - const size_t buffer_pool_size = 5; + const size_t buffer_pool_size = 20; const size_t k = 2; auto disk_manager = std::make_shared(); - auto *bpm = new BufferPoolManager(buffer_pool_size, disk_manager.get(), k); + auto bpm = std::make_shared(buffer_pool_size, disk_manager.get(), k); // Create ten dummy pages, construct a page group - std::vector> p_g; + std::vector p_g; + std::vector p_vec; // Reserve space for p_g, since clang-tidy complains p_g.reserve(10); // Initialize the page group for (int i = 0; i < 10; ++i) { - p_g.push_back(std::make_shared()); + page_id_t tmp_pid = -1; + Page *tmp_page = bpm->NewPage(&tmp_pid); + EXPECT_EQ(tmp_page->GetPinCount(), 1); + EXPECT_NE(tmp_pid, -1); + // Unpin the page + bpm->UnpinPage(tmp_pid, false); + p_g.push_back(tmp_pid); + p_vec.push_back(tmp_page); } // Create a bpg group @@ -133,7 +175,8 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeConcurrentTest1) { // Initialize bpg_g for (int i = 0; i < 10; ++i) { - bpg_g.emplace_back(bpm, p_g[i].get()); + bpg_g.push_back(bpm->FetchPageBasic(p_g[i])); + EXPECT_EQ(bpg_g.back().GetPage()->GetPinCount(), 1); } // Create the thread vector @@ -154,14 +197,18 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeConcurrentTest1) { } // Sanity check - for (const auto &[rpg, index] : rpg_g) { - assert(bpg_g[index].GetPage() == nullptr && bpg_g[index].GetBPM() == nullptr); - assert(rpg.GetPage() == p_g[index].get() && rpg.GetBPM() == bpm); + for (auto &[rpg, index] : rpg_g) { + EXPECT_EQ(bpg_g[index].GetPage(), nullptr); + EXPECT_EQ(bpg_g[index].GetBPM(), nullptr); + EXPECT_EQ(rpg.GetPage(), p_vec[index]); + EXPECT_EQ(rpg.GetBPM(), bpm.get()); + // The pin count should be 1 for each page + EXPECT_EQ(rpg.GetPage()->GetPinCount(), 1); + // Drop the ReadPageGuard + rpg.Drop(); + EXPECT_EQ(p_vec[index]->GetPinCount(), 0); } - // Clean the resource - delete bpm; - // Shutdown the Disk Manager disk_manager->ShutDown(); } @@ -171,9 +218,45 @@ TEST(PageGuardTest, DISABLED_GuardUpgradeConcurrentTest2) { const size_t k = 2; auto disk_manager = std::make_shared(); - auto *bpm = new BufferPoolManager(buffer_pool_size, disk_manager.get(), k); + auto bpm = std::make_shared(buffer_pool_size, disk_manager.get(), k); - + // Create one dummy page + page_id_t p_id = -1; + Page *dummy_page = bpm->NewPage(&p_id); + + // Unpin the page + bpm->UnpinPage(p_id, false); + + // Fetch a ReadPageGuard + ReadPageGuard rpg_1 = bpm->FetchPageRead(p_id); + + // Fetch a second time, assumes this is from another thread + ReadPageGuard rpg_2 = bpm->FetchPageRead(p_id); + + // Current pin count should be 1 + EXPECT_EQ(dummy_page->GetPinCount(), 2); + + // Update the first ReadPageGuard to WritePageGuard in a background thread + // Which should block at present + std::thread t([&]() { + WritePageGuard wpg = rpg_1.UpgradeWrite(); + EXPECT_EQ(wpg.GetPage(), dummy_page); + EXPECT_EQ(dummy_page->GetPinCount(), 1); + + // Drop the WritePageGuard + wpg.Drop(); + EXPECT_EQ(dummy_page->GetPinCount(), 0); + }); + + // Drop the second ReadPageGuard + rpg_2.Drop(); + EXPECT_EQ(rpg_2.GetPage(), nullptr); + + // This should not block + t.join(); + + // Sanity check + EXPECT_EQ(dummy_page->GetPinCount(), 0); } // NOLINTNEXTLINE From 4cb3b8c82953cc9bf6a1765fc6cdc5f360db3568 Mon Sep 17 00:00:00 2001 From: Michael Xu Date: Fri, 29 Sep 2023 03:51:02 -0400 Subject: [PATCH 10/10] update to current interface & remove implementation --- src/include/storage/page/page_guard.h | 40 +++++---------------------- src/storage/page/page_guard.cpp | 35 ++++------------------- 2 files changed, 12 insertions(+), 63 deletions(-) diff --git a/src/include/storage/page/page_guard.h b/src/include/storage/page/page_guard.h index f78c2165e..e2f885f02 100644 --- a/src/include/storage/page/page_guard.h +++ b/src/include/storage/page/page_guard.h @@ -18,32 +18,6 @@ class BasicPageGuard { auto operator=(const BasicPageGuard &) -> BasicPageGuard & = delete; /** TODO(P2): Add implementation - * - * @brief Provide guard upgrade functionality, from BasicPageGuard to ReadPageGuard - * - * You can call this function by: - * BasicPageGuard g; - * ReadPageGuard rg = g.UpgradeRead(); - * Note: You must not use the original BasicPageGuard after calling this function. - * - * @return ReadPageGuard - */ - auto UpgradeRead() -> ReadPageGuard; - - /** TODO(P1): Add implementation - * - * @brief Provide guard upgrade functionality, from BasicPageGuard to WritePageGuard - * - * You can call this function by: - * BasicPageGuard g; - * WritePageGuard wg = g.UpgradeWrite(); - * Note: You must not use the original BasicPageGuard after calling this function. - * - * @return WritePageGuard - */ - auto UpgradeWrite() -> WritePageGuard; - - /** TODO(P1): Add implementation * * @brief Move constructor for BasicPageGuard * @@ -128,12 +102,12 @@ class BasicPageGuard { return reinterpret_cast(GetDataMut()); } - /** Getter Methods */ + /** The following methods are mainly used for testing */ + auto GetPage() const -> Page * { return page_; } auto GetBPM() const -> BufferPoolManager * { return bpm_; } - /** Set the local variables to NULL */ void SetNull() noexcept { page_ = nullptr; bpm_ = nullptr; @@ -168,7 +142,7 @@ class ReadPageGuard { */ auto UpgradeWrite() -> WritePageGuard; - /** TODO(P1): Add implementation + /** TODO(P2): Add implementation * * @brief Move constructor for ReadPageGuard * @@ -216,12 +190,12 @@ class ReadPageGuard { return guard_.As(); } - /** Getter Methods */ + /** The following methods are mainly used for testing */ + auto GetPage() const -> Page * { return guard_.GetPage(); } auto GetBPM() const -> BufferPoolManager * { return guard_.GetBPM(); } - /** Set the guard_'s local variables to NULL */ void SetNull() noexcept { guard_.SetNull(); } private: @@ -291,12 +265,12 @@ class WritePageGuard { return guard_.AsMut(); } - /** Getter Methods */ + /** The following methods are mainly used for testing */ + auto GetPage() const -> Page * { return guard_.GetPage(); } auto GetBPM() const -> BufferPoolManager * { return guard_.GetBPM(); } - /** Set the guard_'s local variables to NULL */ void SetNull() noexcept { guard_.SetNull(); } private: diff --git a/src/storage/page/page_guard.cpp b/src/storage/page/page_guard.cpp index 237e9a919..f81a0e34e 100644 --- a/src/storage/page/page_guard.cpp +++ b/src/storage/page/page_guard.cpp @@ -4,43 +4,18 @@ namespace bustub { auto BasicPageGuard::UpgradeRead() -> ReadPageGuard { - // Basic sanity check - BUSTUB_ENSURE((bpm_ != nullptr && page_ != nullptr), "The BPM & Page in the current guard must not be NULL"); - // Manually lock the underlying page with read lock - page_->RLatch(); - // Construct the newly returned `ReadPageGuard` - ReadPageGuard ret_rpg = ReadPageGuard{bpm_, page_}; - // Set the local variables to null - SetNull(); - // Return the rvalue, will invoke move assignment operator - return ret_rpg; + // TODO(p2): Your ReadPageGuard upgrade logic here + return {nullptr, nullptr}; } auto BasicPageGuard::UpgradeWrite() -> WritePageGuard { - // TODO(p1): Your WritePageGuard upgrade logic here + // TODO(p2): Your WritePageGuard upgrade logic here return {nullptr, nullptr}; } auto ReadPageGuard::UpgradeWrite() -> WritePageGuard { - // Get the underlying page from guard_ - Page *cur_page = guard_.GetPage(); - // Perform sanity check - BUSTUB_ENSURE((cur_page != nullptr), "The Page in the guard_ must not be NULL"); - // Release the current read lock (Since the pin count is not changed, the current page will not be evicted) - // But I think the semantic here may be further checked, since this operation is not actually `atomic` to user - // e.g, There may be other thread wanting to acquire write lock at the same time, - // also some read operations may happen concurrently, causing potential blocking - // P.S. The std::shared_mutex doesn't officially support atomic lock upgrade :( - // Boost Lib has some of the functionalities(atomic mutex upgrade), but it's not allowed here though - cur_page->RUnlatch(); - // Acquire new write lock - cur_page->WLatch(); - // Construct a newly returned `WritePageGuard` - WritePageGuard ret_wpg = WritePageGuard{guard_.GetBPM(), guard_.GetPage()}; - // Set the local variables to null - guard_.SetNull(); - // Return the rvalue - return ret_wpg; + // TODO(p2): Your WritePageGuard upgrade logic here + return {nullptr, nullptr}; } BasicPageGuard::BasicPageGuard(BasicPageGuard &&that) noexcept {}