From 26345e2ab9cecb96f0f47aedf054c469d2188be0 Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Fri, 21 Mar 2025 14:50:02 +0100 Subject: [PATCH 01/11] Add ThreadOwnedList --- lib/CMakeLists.txt | 1 + lib/Containers/CMakeLists.txt | 1 + lib/Containers/Concurrent/CMakeLists.txt | 7 + lib/Containers/Concurrent/ThreadOwnedList.cpp | 179 +++++++++++++ lib/Containers/Concurrent/ThreadOwnedList.h | 238 ++++++++++++++++ lib/Containers/Concurrent/thread.cpp | 35 +++ lib/Containers/Concurrent/thread.h | 42 +++ tests/CMakeLists.txt | 1 + tests/Containers/CMakeLists.txt | 1 + tests/Containers/Concurrent/CMakeLists.txt | 5 + .../Concurrent/ThreadOwnedListTest.cpp | 253 ++++++++++++++++++ 11 files changed, 763 insertions(+) create mode 100644 lib/Containers/CMakeLists.txt create mode 100644 lib/Containers/Concurrent/CMakeLists.txt create mode 100644 lib/Containers/Concurrent/ThreadOwnedList.cpp create mode 100644 lib/Containers/Concurrent/ThreadOwnedList.h create mode 100644 lib/Containers/Concurrent/thread.cpp create mode 100644 lib/Containers/Concurrent/thread.h create mode 100644 tests/Containers/CMakeLists.txt create mode 100644 tests/Containers/Concurrent/CMakeLists.txt create mode 100644 tests/Containers/Concurrent/ThreadOwnedListTest.cpp diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index cf08d2d919e1..dd04f5f1488c 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -440,6 +440,7 @@ endif() add_subdirectory(Actor) add_subdirectory(Async) +add_subdirectory(Containers) add_subdirectory(Futures) add_subdirectory(Geo) if (USE_V8) diff --git a/lib/Containers/CMakeLists.txt b/lib/Containers/CMakeLists.txt new file mode 100644 index 000000000000..3f37c693cf96 --- /dev/null +++ b/lib/Containers/CMakeLists.txt @@ -0,0 +1 @@ +add_subdirectory(Concurrent) diff --git a/lib/Containers/Concurrent/CMakeLists.txt b/lib/Containers/Concurrent/CMakeLists.txt new file mode 100644 index 000000000000..03d17fda610b --- /dev/null +++ b/lib/Containers/Concurrent/CMakeLists.txt @@ -0,0 +1,7 @@ +add_library(arango_thread_owning_list STATIC + thread.cpp) +target_include_directories(arango_thread_owning_list PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) +target_link_libraries(arango_thread_owning_list + PRIVATE + arango_basic_utils + arango_inspection) diff --git a/lib/Containers/Concurrent/ThreadOwnedList.cpp b/lib/Containers/Concurrent/ThreadOwnedList.cpp new file mode 100644 index 000000000000..db87aa7a36f2 --- /dev/null +++ b/lib/Containers/Concurrent/ThreadOwnedList.cpp @@ -0,0 +1,179 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#include "ThreadOwnedList.h" + +#include "fmt/core.h" + +using namespace arangodb::containers; + +template +auto ThreadOwnedList::make() noexcept + -> std::shared_ptr> { + struct MakeShared : ThreadOwnedList { + MakeShared() : ThreadOwnedList() {} + }; + return std::make_shared(); +} + +template +ThreadOwnedList::ThreadOwnedList() noexcept + : thread{basics::ThreadId::current()} {} + +template +ThreadOwnedList::~ThreadOwnedList() noexcept { + cleanup(); +} + +template +auto ThreadOwnedList::add(T data) noexcept -> Node* { + auto current_thread = basics::ThreadId::current(); + ADB_PROD_ASSERT(current_thread == thread) << fmt::format( + "ThreadOwnedList::add_promise was called from thread {} but needs to be " + "called from ThreadOwnedList's owning thread {}. {}", + current_thread, thread, this); + auto current_head = _head.load(std::memory_order_relaxed); + auto node = new Node{.data = std::move(data), + .next = current_head, + .list = this->shared_from_this()}; + if (current_head != nullptr) { + current_head->previous.store(node); // TODO memory order + } + // (1) - this store synchronizes with load in (2) + _head.store(node, std::memory_order_release); + metrics.increment_registered_nodes(); + metrics.increment_total_nodes(); + return node; +} + +template +auto ThreadOwnedList::mark_for_deletion(Node* node) noexcept -> void { + // makes sure that promise is really in this list + ADB_PROD_ASSERT(node->registry.get() == this); + + // TODO needs to be done in Promise::mark_for_deletion instead + // promise->state.store(State::Deleted); + + // keep a local copy of the shared pointer. This promise might be the + // last of the registry. + auto self = std::move(node->registry); + + auto current_head = _free_head.load(std::memory_order_relaxed); + do { + node->next_to_free = current_head; + // (4) - this compare_exchange_weak synchronizes with exchange in (5) + } while (not _free_head.compare_exchange_weak(current_head, node, + std::memory_order_release, + std::memory_order_acquire)); + // DO NOT access promise after this line. The owner thread might already + // be running a cleanup and promise might be deleted. + + metrics.decrement_registered_nodes(); + metrics.increment_ready_for_deletion_nodes(); + + // self destroyed here. registry might be destroyed here as well. +} + +template +auto ThreadOwnedList::garbage_collect() noexcept -> void { + auto current_thread = basics::ThreadId::current(); + ADB_PROD_ASSERT(current_thread == thread) << fmt::format( + "ThreadOwnedList::garbage_collect was called from thread {} but needs to " + "be called from ThreadOwnedList's owning thread {}. {}", + current_thread, thread, this); + auto guard = std::lock_guard(_mutex); + cleanup(); +} + +template +auto ThreadOwnedList::garbage_collect_external() noexcept -> void { + // acquire the lock. This prevents the owning thread and the observer + // from accessing promises. Note that the owing thread only adds new + // promises to the head of the list. + auto guard = std::lock_guard(_mutex); + // we can make the following observation. Once a promise is enqueued in the + // list, it previous and next pointer is never updated, except for the current + // head element. Also, Promises are only removed, after the mutex has been + // acquired. This implies that we can clean up all promises, that are not + // in head position right now. + Node* maybe_head_ptr = nullptr; + Node*current, + *next = _free_head.exchange( + nullptr, std::memory_order_acquire); // TODO check memory order + while (next != nullptr) { + current = next; + next = next->next_to_free; + if (current->previous != nullptr) { // TODO memory order + metrics.decrement_ready_for_deletion_nodes(); + remove(current); + delete current; + } else { + // if this is the head of the promise list, we cannot delete it because + // additional promises could have been added in the meantime + // (if these new promises would have been marked in the meantime, they + // would be in the new free list due to the exchange earlier) + ADB_PROD_ASSERT(maybe_head_ptr == nullptr); + maybe_head_ptr = current; + } + } + // After the clean up we have to add the potential head back into the free + // list. + if (maybe_head_ptr) { + auto current_head = _free_head.load(std::memory_order_relaxed); + do { + maybe_head_ptr->next_to_free = current_head; + // (4) - this compare_exchange_weak synchronizes with exchange in (5) + // TODO check memory order + } while (not _free_head.compare_exchange_weak(current_head, maybe_head_ptr, + std::memory_order_release, + std::memory_order_acquire)); + } +} + +template +auto ThreadOwnedList::cleanup() noexcept -> void { + // (5) - this exchange synchronizes with compare_exchange_weak in (4) + Node*current, + *next = _free_head.exchange(nullptr, std::memory_order_acquire); + while (next != nullptr) { + current = next; + next = next->next_to_free; + metrics.decrement_ready_for_deletion_nodes(); + remove(current); + delete current; + } +} + +template +auto ThreadOwnedList::remove(Node* node) -> void { + auto* next = node->next; + auto* previous = node->previous.load(); // TODO memory order + if (previous == nullptr) { // promise is current head + // (3) - this store synchronizes with the load in (2) + _head.store(next, std::memory_order_release); + } else { + previous->next = next; + } + if (next != nullptr) { + next->previous = previous; // TODO memory order + } +} diff --git a/lib/Containers/Concurrent/ThreadOwnedList.h b/lib/Containers/Concurrent/ThreadOwnedList.h new file mode 100644 index 000000000000..6203f23c4857 --- /dev/null +++ b/lib/Containers/Concurrent/ThreadOwnedList.h @@ -0,0 +1,238 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#pragma once + +#include "Containers/Concurrent/thread.h" +#include "Inspection/Format.h" +#include "fmt/core.h" + +#include +#include + +namespace arangodb::containers { + +template +concept HasSnapshot = requires(T t) { + { t.snapshot() } -> std::convertible_to; +}; +template +concept DefinesMetrics = requires(M m) { + m.increment_total_nodes(); + m.increment_registered_nodes(); + m.decrement_registered_nodes(); + m.increment_ready_for_deletion_nodes(); + m.decrement_ready_for_deletion_nodes(); +}; + +template +struct ThreadOwnedList; + +template +struct Node { + T data; + Node* next = nullptr; + std::atomic previous = + nullptr; // needs to be atomic for gc from different thread + Node* next_to_free = nullptr; + std::shared_ptr> + list; // to be able to mark for deletion +}; + +/** + This list is owned by one thread: nodes can only be added on this thread but + other threads can read the list and mark nodes for deletion. + + Nodes have to manually be marked for deletion, otherwise nodes are not + deleted and therfore also not this list because each node includes a + shared_ptr to this list. Garbage collection can run either on the owning + thread or on another thread (there are two distinct functions for gc). + */ +template // T requires a Snapshot type +struct ThreadOwnedList + : public std::enable_shared_from_this> { + basics::ThreadId const thread; + + private: + std::atomic*> _head = nullptr; + std::atomic*> _free_head = nullptr; + std::mutex _mutex; // gc and reading cannot happen at same time (perhaps I + // can get rid of this with epoch based reclamation) + M metrics; + + public: + static auto make() noexcept -> std::shared_ptr { + struct MakeShared : ThreadOwnedList { + MakeShared() : ThreadOwnedList() {} + }; + return std::make_shared(); + } + + ~ThreadOwnedList() noexcept { cleanup(); } + template + requires std::invocable + auto for_promise(F&& function) noexcept -> void { + auto guard = std::lock_guard(_mutex); + // (2) - this load synchronizes with store in (1) and (3) + for (auto current = _head.load(std::memory_order_acquire); + current != nullptr; current = current->next) { + function(current->data.snapshot()); + } + } + + auto add(T data) noexcept -> Node* { + auto current_thread = basics::ThreadId::current(); + ADB_PROD_ASSERT(current_thread == thread) << fmt::format( + "ThreadOwnedList::add_promise was called from thread {} but needs to " + "be " + "called from ThreadOwnedList's owning thread {}. {}", + inspection::json(current_thread), inspection::json(thread), + (void*)this); + auto current_head = _head.load(std::memory_order_relaxed); + auto node = new Node{.data = std::move(data), + .next = current_head, + .list = this->shared_from_this()}; + if (current_head != nullptr) { + current_head->previous.store(node); // TODO memory order + } + // (1) - this store synchronizes with load in (2) + _head.store(node, std::memory_order_release); + metrics.increment_registered_nodes(); + metrics.increment_total_nodes(); + return node; + } + + auto mark_for_deletion(Node* node) noexcept -> void { + // makes sure that promise is really in this list + ADB_PROD_ASSERT(node->list.get() == this); + + // TODO needs to be done in Promise::mark_for_deletion instead + // promise->state.store(State::Deleted); + + // keep a local copy of the shared pointer. This promise might be the + // last of the registry. + auto self = std::move(node->list); + + auto current_head = _free_head.load(std::memory_order_relaxed); + do { + node->next_to_free = current_head; + // (4) - this compare_exchange_weak synchronizes with exchange in (5) + } while (not _free_head.compare_exchange_weak(current_head, node, + std::memory_order_release, + std::memory_order_acquire)); + // DO NOT access promise after this line. The owner thread might already + // be running a cleanup and promise might be deleted. + + metrics.decrement_registered_nodes(); + metrics.increment_ready_for_deletion_nodes(); + + // self destroyed here. registry might be destroyed here as well. + } + + auto garbage_collect() noexcept -> void { + auto current_thread = basics::ThreadId::current(); + ADB_PROD_ASSERT(current_thread == thread) << fmt::format( + "ThreadOwnedList::garbage_collect was called from thread {} but needs " + "to " + "be called from ThreadOwnedList's owning thread {}. {}", + inspection::json(current_thread), inspection::json(thread), + (void*)this); + auto guard = std::lock_guard(_mutex); + cleanup(); + } + + auto garbage_collect_external() noexcept -> void { + // acquire the lock. This prevents the owning thread and the observer + // from accessing promises. Note that the owing thread only adds new + // promises to the head of the list. + auto guard = std::lock_guard(_mutex); + // we can make the following observation. Once a promise is enqueued in the + // list, it previous and next pointer is never updated, except for the + // current head element. Also, Promises are only removed, after the mutex + // has been acquired. This implies that we can clean up all promises, that + // are not in head position right now. + Node* maybe_head_ptr = nullptr; + Node*current, + *next = _free_head.exchange( + nullptr, std::memory_order_acquire); // TODO check memory order + while (next != nullptr) { + current = next; + next = next->next_to_free; + if (current->previous != nullptr) { // TODO memory order + metrics.decrement_ready_for_deletion_nodes(); + remove(current); + delete current; + } else { + // if this is the head of the promise list, we cannot delete it because + // additional promises could have been added in the meantime + // (if these new promises would have been marked in the meantime, they + // would be in the new free list due to the exchange earlier) + ADB_PROD_ASSERT(maybe_head_ptr == nullptr); + maybe_head_ptr = current; + } + } + // After the clean up we have to add the potential head back into the free + // list. + if (maybe_head_ptr) { + auto current_head = _free_head.load(std::memory_order_relaxed); + do { + maybe_head_ptr->next_to_free = current_head; + // (4) - this compare_exchange_weak synchronizes with exchange in (5) + // TODO check memory order + } while (not _free_head.compare_exchange_weak( + current_head, maybe_head_ptr, std::memory_order_release, + std::memory_order_acquire)); + } + } + + private: + ThreadOwnedList() noexcept : thread{basics::ThreadId::current()} {} + + auto cleanup() noexcept -> void { + // (5) - this exchange synchronizes with compare_exchange_weak in (4) + Node*current, + *next = _free_head.exchange(nullptr, std::memory_order_acquire); + while (next != nullptr) { + current = next; + next = next->next_to_free; + metrics.decrement_ready_for_deletion_nodes(); + remove(current); + delete current; + } + } + + auto remove(Node* node) -> void { + auto* next = node->next; + auto* previous = node->previous.load(); // TODO memory order + if (previous == nullptr) { // promise is current head + // (3) - this store synchronizes with the load in (2) + _head.store(next, std::memory_order_release); + } else { + previous->next = next; + } + if (next != nullptr) { + next->previous = previous; // TODO memory order + } + } +}; + +} // namespace arangodb::containers diff --git a/lib/Containers/Concurrent/thread.cpp b/lib/Containers/Concurrent/thread.cpp new file mode 100644 index 000000000000..12c87de8c13b --- /dev/null +++ b/lib/Containers/Concurrent/thread.cpp @@ -0,0 +1,35 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#include "thread.h" + +#include "Basics/Thread.h" + +using namespace arangodb::basics; + +auto ThreadId::current() noexcept -> ThreadId { + return ThreadId{.posix_id = arangodb::Thread::currentThreadId(), + .kernel_id = arangodb::Thread::currentKernelThreadId()}; +} +auto ThreadId::name() -> std::string { + return std::string{ThreadNameFetcher{posix_id}.get()}; +} diff --git a/lib/Containers/Concurrent/thread.h b/lib/Containers/Concurrent/thread.h new file mode 100644 index 000000000000..2a2231d08efc --- /dev/null +++ b/lib/Containers/Concurrent/thread.h @@ -0,0 +1,42 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#pragma once + +#include "Basics/threads-posix.h" + +namespace arangodb::basics { + +struct ThreadId { + static auto current() noexcept -> ThreadId; + auto name() -> std::string; + TRI_tid_t posix_id; + pid_t kernel_id; + bool operator==(ThreadId const&) const = default; +}; +template +auto inspect(Inspector& f, ThreadId& x) { + return f.object(x).fields(f.field("LWPID", x.kernel_id), + f.field("name", x.name())); +} + +} // namespace arangodb::basics diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 01c06403664f..e1163282254b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -420,5 +420,6 @@ endforeach() add_subdirectory(Actor) add_subdirectory(Async) add_subdirectory(AsyncRegistryServer) +add_subdirectory(Containers) add_subdirectory(sepp) add_subdirectory(VocBase/Properties) diff --git a/tests/Containers/CMakeLists.txt b/tests/Containers/CMakeLists.txt new file mode 100644 index 000000000000..3f37c693cf96 --- /dev/null +++ b/tests/Containers/CMakeLists.txt @@ -0,0 +1 @@ +add_subdirectory(Concurrent) diff --git a/tests/Containers/Concurrent/CMakeLists.txt b/tests/Containers/Concurrent/CMakeLists.txt new file mode 100644 index 000000000000..5c63a2fca634 --- /dev/null +++ b/tests/Containers/Concurrent/CMakeLists.txt @@ -0,0 +1,5 @@ +target_sources(arangodbtests + PRIVATE + ThreadOwnedListTest.cpp) +target_link_libraries(arangodbtests + arango_thread_owning_list) diff --git a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp new file mode 100644 index 000000000000..ff504a26eac3 --- /dev/null +++ b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp @@ -0,0 +1,253 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#include "Containers/Concurrent/ThreadOwnedList.h" + +#include +#include +#include +#include + +using namespace arangodb::containers; + +namespace { + +struct InstanceCounterValue { + InstanceCounterValue() { instanceCounter += 1; } + InstanceCounterValue(InstanceCounterValue const& o) { instanceCounter += 1; } + InstanceCounterValue(InstanceCounterValue&& o) noexcept { + instanceCounter += 1; + } + + ~InstanceCounterValue() { + if (instanceCounter == 0) { + abort(); + } + instanceCounter -= 1; + } + static std::size_t instanceCounter; +}; +std::size_t InstanceCounterValue::instanceCounter = 0; + +struct NodeData : InstanceCounterValue { + int number; + + NodeData(int number) : number{number} {} + + struct Snapshot { + int number; + bool operator==(Snapshot const&) const = default; + }; + auto snapshot() -> Snapshot { return Snapshot{.number = number}; } +}; +struct Metrics { + auto increment_total_nodes() -> void{}; + auto increment_registered_nodes() -> void{}; + auto decrement_registered_nodes() -> void{}; + auto increment_ready_for_deletion_nodes() -> void{}; + auto decrement_ready_for_deletion_nodes() -> void{}; +}; + +using MyList = ThreadOwnedList; + +auto nodes_in_registry(std::shared_ptr registry) + -> std::vector { + std::vector promises; + registry->for_promise( + [&](NodeData::Snapshot promise) { promises.push_back(promise); }); + return promises; +} +} // namespace + +struct ThreadOwnedListTest : ::testing::Test { + void TearDown() override { + EXPECT_EQ(InstanceCounterValue::instanceCounter, 0); + } +}; +using ThreadOwnedListDeathTest = ThreadOwnedListTest; + +TEST_F(ThreadOwnedListTest, adds_a_promise) { + auto registry = MyList::make(); + + auto node = registry->add(NodeData{2}); + + EXPECT_EQ(nodes_in_registry(registry), + (std::vector{node->data.snapshot()})); + + // make sure registry is cleaned up + registry->mark_for_deletion(node); +} + +TEST_F(ThreadOwnedListDeathTest, another_thread_cannot_add_a_promise) { + GTEST_FLAG_SET(death_test_style, "threadsafe"); + auto registry = MyList::make(); + + std::jthread( + [&]() { EXPECT_DEATH(registry->add(NodeData{1}), "Assertion failed"); }); +} + +TEST_F(ThreadOwnedListTest, iterates_over_all_promises) { + auto registry = MyList::make(); + + auto* first_node = registry->add(NodeData{5}); + auto* second_node = registry->add(NodeData{9}); + auto* third_node = registry->add(NodeData{10}); + + EXPECT_EQ(nodes_in_registry(registry), + (std::vector{third_node->data.snapshot(), + second_node->data.snapshot(), + first_node->data.snapshot()})); + + // make sure registry is cleaned up + registry->mark_for_deletion(first_node); + registry->mark_for_deletion(second_node); + registry->mark_for_deletion(third_node); +} + +TEST_F(ThreadOwnedListTest, iterates_in_another_thread_over_all_promises) { + auto registry = MyList::make(); + + auto* first_node = registry->add(NodeData{19}); + auto* second_node = registry->add(NodeData{0}); + auto* third_node = registry->add(NodeData{3}); + + std::thread([&]() { + EXPECT_EQ(nodes_in_registry(registry), + (std::vector{third_node->data.snapshot(), + second_node->data.snapshot(), + first_node->data.snapshot()})); + }).join(); + + // make sure registry is cleaned up + registry->mark_for_deletion(first_node); + registry->mark_for_deletion(second_node); + registry->mark_for_deletion(third_node); +} + +TEST_F(ThreadOwnedListTest, marked_promises_are_deleted_in_garbage_collection) { + auto registry = MyList::make(); + auto* node_to_delete = registry->add(NodeData{1}); + auto* another_node = registry->add(NodeData{77}); + + registry->mark_for_deletion(node_to_delete); + EXPECT_EQ(nodes_in_registry(registry), + (std::vector{another_node->data.snapshot(), + node_to_delete->data.snapshot()})); + + registry->garbage_collect(); + EXPECT_EQ(nodes_in_registry(registry), + (std::vector{another_node->data.snapshot()})); + + // make sure registry is cleaned up + registry->mark_for_deletion(another_node); +} + +TEST_F(ThreadOwnedListTest, garbage_collection_deletes_marked_promises) { + { + auto registry = MyList::make(); + auto* first_node = registry->add(NodeData{21}); + auto* second_node = registry->add(NodeData{1}); + auto* third_node = registry->add(NodeData{100}); + + registry->mark_for_deletion(first_node); + registry->garbage_collect(); + + EXPECT_EQ(nodes_in_registry(registry), + (std::vector{third_node->data.snapshot(), + second_node->data.snapshot()})); + + // clean up + registry->mark_for_deletion(second_node); + registry->mark_for_deletion(third_node); + } + { + auto registry = MyList::make(); + auto* first_node = registry->add(NodeData{21}); + auto* second_node = registry->add(NodeData{1}); + auto* third_node = registry->add(NodeData{100}); + + registry->mark_for_deletion(second_node); + registry->garbage_collect(); + + EXPECT_EQ(nodes_in_registry(registry), + (std::vector{third_node->data.snapshot(), + first_node->data.snapshot()})); + + // clean up + registry->mark_for_deletion(first_node); + registry->mark_for_deletion(third_node); + } + { + auto registry = MyList::make(); + auto* first_node = registry->add(NodeData{21}); + auto* second_node = registry->add(NodeData{1}); + auto* third_node = registry->add(NodeData{100}); + + registry->mark_for_deletion(third_node); + registry->garbage_collect(); + + EXPECT_EQ(nodes_in_registry(registry), + (std::vector{second_node->data.snapshot(), + first_node->data.snapshot()})); + + // clean up + registry->mark_for_deletion(first_node); + registry->mark_for_deletion(second_node); + } +} + +TEST_F(ThreadOwnedListDeathTest, + unrelated_promise_cannot_be_marked_for_deletion) { + GTEST_FLAG_SET(death_test_style, "threadsafe"); + auto registry = MyList::make(); + auto some_other_registry = MyList::make(); + + auto* promise = some_other_registry->add(NodeData{33}); + + EXPECT_DEATH(registry->mark_for_deletion(promise), "Assertion failed"); +} + +TEST_F(ThreadOwnedListTest, another_thread_can_mark_a_promise_for_deletion) { + auto registry = MyList::make(); + + auto* node_to_delete = registry->add(NodeData{7}); + auto* another_node = registry->add(NodeData{4}); + + std::thread([&]() { registry->mark_for_deletion(node_to_delete); }).join(); + + registry->garbage_collect(); + EXPECT_EQ(nodes_in_registry(registry), + (std::vector{another_node->data.snapshot()})); + + // clean up + registry->mark_for_deletion(another_node); +} + +TEST_F(ThreadOwnedListDeathTest, + garbage_collection_cannot_be_called_on_different_thread) { + GTEST_FLAG_SET(death_test_style, "threadsafe"); + + auto registry = MyList::make(); + + std::jthread( + [&] { EXPECT_DEATH(registry->garbage_collect(), "Assertion failed"); }); +} From 8e805ecac2d9ed12989fed9a9b7a2eb68cdcb915 Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Mon, 24 Mar 2025 19:14:34 +0100 Subject: [PATCH 02/11] Add list of lists structure --- .../Concurrent/ListOfThreadOwnedLists.h | 74 +++++++++++++++ lib/Containers/Concurrent/ThreadOwnedList.h | 54 +++++------ lib/Containers/Concurrent/snapshot.h | 30 ++++++ tests/Containers/Concurrent/CMakeLists.txt | 3 +- .../Containers/Concurrent/ListOfListsTest.cpp | 92 +++++++++++++++++++ .../Concurrent/ThreadOwnedListTest.cpp | 2 +- 6 files changed, 223 insertions(+), 32 deletions(-) create mode 100644 lib/Containers/Concurrent/ListOfThreadOwnedLists.h create mode 100644 lib/Containers/Concurrent/snapshot.h create mode 100644 tests/Containers/Concurrent/ListOfListsTest.cpp diff --git a/lib/Containers/Concurrent/ListOfThreadOwnedLists.h b/lib/Containers/Concurrent/ListOfThreadOwnedLists.h new file mode 100644 index 000000000000..1e22e8860579 --- /dev/null +++ b/lib/Containers/Concurrent/ListOfThreadOwnedLists.h @@ -0,0 +1,74 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#pragma once + +#include "Containers/Concurrent/snapshot.h" + +#include +#include + +namespace arangodb::containers { + +template +concept IteratorOverNodes = requires(List l, F f) { + l.for_node(f); +}; +template +concept IteratorOverSnapshots = + IteratorOverNodes && std::invocable; + +template +struct ListOfLists { + private: + std::vector> _lists; + std::mutex _mutex; + // std::shared_ptr _metrics; // TODO + + public: + // ListOfLists(); + auto add(std::shared_ptr list) -> void { + auto guard = std::lock_guard(_mutex); + // make sure that expired nodes are deleted + std::erase_if(_lists, [&](auto const& list) { return list.expired(); }); + _lists.emplace_back(list); + } + + template + requires IteratorOverSnapshots + auto for_node(F&& function) -> void { + auto lists = [&] { + auto guard = std::lock_guard(_mutex); + return _lists; + }(); + + for (auto& weak_list : _lists) { + if (auto list = weak_list.lock()) { + list->for_node(function); + } + } + } + // auto set_metrics(std::shared_ptr metrics) -> void; // TODO + // void run_external_cleanup() noexcept; +}; + +} // namespace arangodb::containers diff --git a/lib/Containers/Concurrent/ThreadOwnedList.h b/lib/Containers/Concurrent/ThreadOwnedList.h index 6203f23c4857..5b6c2e8c5627 100644 --- a/lib/Containers/Concurrent/ThreadOwnedList.h +++ b/lib/Containers/Concurrent/ThreadOwnedList.h @@ -23,6 +23,7 @@ #pragma once #include "Containers/Concurrent/thread.h" +#include "Containers/Concurrent/snapshot.h" #include "Inspection/Format.h" #include "fmt/core.h" @@ -31,10 +32,6 @@ namespace arangodb::containers { -template -concept HasSnapshot = requires(T t) { - { t.snapshot() } -> std::convertible_to; -}; template concept DefinesMetrics = requires(M m) { m.increment_total_nodes(); @@ -44,20 +41,6 @@ concept DefinesMetrics = requires(M m) { m.decrement_ready_for_deletion_nodes(); }; -template -struct ThreadOwnedList; - -template -struct Node { - T data; - Node* next = nullptr; - std::atomic previous = - nullptr; // needs to be atomic for gc from different thread - Node* next_to_free = nullptr; - std::shared_ptr> - list; // to be able to mark for deletion -}; - /** This list is owned by one thread: nodes can only be added on this thread but other threads can read the list and mark nodes for deletion. @@ -72,9 +55,19 @@ struct ThreadOwnedList : public std::enable_shared_from_this> { basics::ThreadId const thread; + struct Node { + T data; + Node* next = nullptr; + std::atomic previous = + nullptr; // needs to be atomic for gc from different thread + Node* next_to_free = nullptr; + std::shared_ptr> + list; // to be able to mark for deletion + }; + private: - std::atomic*> _head = nullptr; - std::atomic*> _free_head = nullptr; + std::atomic _head = nullptr; + std::atomic _free_head = nullptr; std::mutex _mutex; // gc and reading cannot happen at same time (perhaps I // can get rid of this with epoch based reclamation) M metrics; @@ -88,9 +81,10 @@ struct ThreadOwnedList } ~ThreadOwnedList() noexcept { cleanup(); } + template requires std::invocable - auto for_promise(F&& function) noexcept -> void { + auto for_node(F&& function) noexcept -> void { auto guard = std::lock_guard(_mutex); // (2) - this load synchronizes with store in (1) and (3) for (auto current = _head.load(std::memory_order_acquire); @@ -99,7 +93,7 @@ struct ThreadOwnedList } } - auto add(T data) noexcept -> Node* { + auto add(T data) noexcept -> Node* { auto current_thread = basics::ThreadId::current(); ADB_PROD_ASSERT(current_thread == thread) << fmt::format( "ThreadOwnedList::add_promise was called from thread {} but needs to " @@ -108,9 +102,9 @@ struct ThreadOwnedList inspection::json(current_thread), inspection::json(thread), (void*)this); auto current_head = _head.load(std::memory_order_relaxed); - auto node = new Node{.data = std::move(data), - .next = current_head, - .list = this->shared_from_this()}; + auto node = new Node{.data = std::move(data), + .next = current_head, + .list = this->shared_from_this()}; if (current_head != nullptr) { current_head->previous.store(node); // TODO memory order } @@ -121,7 +115,7 @@ struct ThreadOwnedList return node; } - auto mark_for_deletion(Node* node) noexcept -> void { + auto mark_for_deletion(Node* node) noexcept -> void { // makes sure that promise is really in this list ADB_PROD_ASSERT(node->list.get() == this); @@ -170,8 +164,8 @@ struct ThreadOwnedList // current head element. Also, Promises are only removed, after the mutex // has been acquired. This implies that we can clean up all promises, that // are not in head position right now. - Node* maybe_head_ptr = nullptr; - Node*current, + Node* maybe_head_ptr = nullptr; + Node *current, *next = _free_head.exchange( nullptr, std::memory_order_acquire); // TODO check memory order while (next != nullptr) { @@ -209,7 +203,7 @@ struct ThreadOwnedList auto cleanup() noexcept -> void { // (5) - this exchange synchronizes with compare_exchange_weak in (4) - Node*current, + Node *current, *next = _free_head.exchange(nullptr, std::memory_order_acquire); while (next != nullptr) { current = next; @@ -220,7 +214,7 @@ struct ThreadOwnedList } } - auto remove(Node* node) -> void { + auto remove(Node* node) -> void { auto* next = node->next; auto* previous = node->previous.load(); // TODO memory order if (previous == nullptr) { // promise is current head diff --git a/lib/Containers/Concurrent/snapshot.h b/lib/Containers/Concurrent/snapshot.h new file mode 100644 index 000000000000..59ac31d411cb --- /dev/null +++ b/lib/Containers/Concurrent/snapshot.h @@ -0,0 +1,30 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#pragma once + +#include + +template +concept HasSnapshot = requires(T t) { + { t.snapshot() } -> std::convertible_to; +}; diff --git a/tests/Containers/Concurrent/CMakeLists.txt b/tests/Containers/Concurrent/CMakeLists.txt index 5c63a2fca634..b646bc654b59 100644 --- a/tests/Containers/Concurrent/CMakeLists.txt +++ b/tests/Containers/Concurrent/CMakeLists.txt @@ -1,5 +1,6 @@ target_sources(arangodbtests PRIVATE - ThreadOwnedListTest.cpp) + ThreadOwnedListTest.cpp + ListOfListsTest.cpp) target_link_libraries(arangodbtests arango_thread_owning_list) diff --git a/tests/Containers/Concurrent/ListOfListsTest.cpp b/tests/Containers/Concurrent/ListOfListsTest.cpp new file mode 100644 index 000000000000..cb77dc2ccea7 --- /dev/null +++ b/tests/Containers/Concurrent/ListOfListsTest.cpp @@ -0,0 +1,92 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#include "Containers/Concurrent/ListOfThreadOwnedLists.h" + +#include +#include + +using namespace arangodb::containers; + +namespace { + +struct MyData { + int number; + MyData(int number) : number{number} {} + struct Snapshot { + int number; + bool operator==(Snapshot const&) const = default; + }; + auto snapshot() const -> Snapshot { return Snapshot{.number = number}; } +}; + +struct MyNodeList { + std::vector data; + + template + requires std::invocable + auto for_node(F&& function) -> void { + for (auto const& item : data) { + function(item.snapshot()); + } + } +}; + +using MyList = ListOfLists; + +auto nodes_in_list(MyList& registry) -> std::vector { + std::vector nodes; + registry.for_node([&](MyData::Snapshot node) { nodes.push_back(node); }); + return nodes; +} +} // namespace + +TEST(ListOfListsTest, registers_a_list) { + MyList list; + auto inner_list = std::make_shared(std::vector{1, 3, 4}); + + list.add(inner_list); + + EXPECT_EQ(nodes_in_list(list), + (std::vector{{1}, {3}, {4}})); +} + +TEST(ListOfListsTest, does_not_extend_lifetime_of_internal_list) { + MyList list; + + list.add(std::make_shared(std::vector{1, 3, 4})); + + EXPECT_EQ(nodes_in_list(list), (std::vector{})); +} + +TEST(ListOfListsTest, iterates_over_list_items) { + MyList list; + auto first_inner_list = + std::make_shared(std::vector{1, 2, 3}); + auto second_inner_list = + std::make_shared(std::vector{4, 5, 6}); + list.add(first_inner_list); + list.add(second_inner_list); + + EXPECT_EQ(nodes_in_list(list), + (std::vector{{1}, {2}, {3}, {4}, {5}, {6}})); +} diff --git a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp index ff504a26eac3..84ceefb4bcc6 100644 --- a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp +++ b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp @@ -72,7 +72,7 @@ using MyList = ThreadOwnedList; auto nodes_in_registry(std::shared_ptr registry) -> std::vector { std::vector promises; - registry->for_promise( + registry->for_node( [&](NodeData::Snapshot promise) { promises.push_back(promise); }); return promises; } From e5f47f1ae6f6de4091e5da990f330e92e8480ef1 Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Mon, 24 Mar 2025 20:34:10 +0100 Subject: [PATCH 03/11] Make metrics type dynamic --- .../Concurrent/ListOfThreadOwnedLists.h | 18 ++++- lib/Containers/Concurrent/ThreadOwnedList.h | 67 ++++++++++++------- lib/Containers/Concurrent/metrics.h | 46 +++++++++++++ .../Containers/Concurrent/ListOfListsTest.cpp | 45 +++++++++++++ .../Concurrent/ThreadOwnedListTest.cpp | 9 +-- 5 files changed, 150 insertions(+), 35 deletions(-) create mode 100644 lib/Containers/Concurrent/metrics.h diff --git a/lib/Containers/Concurrent/ListOfThreadOwnedLists.h b/lib/Containers/Concurrent/ListOfThreadOwnedLists.h index 1e22e8860579..d54ee9e423ca 100644 --- a/lib/Containers/Concurrent/ListOfThreadOwnedLists.h +++ b/lib/Containers/Concurrent/ListOfThreadOwnedLists.h @@ -23,6 +23,7 @@ #pragma once #include "Containers/Concurrent/snapshot.h" +#include "Containers/Concurrent/metrics.h" #include #include @@ -37,17 +38,24 @@ template concept IteratorOverSnapshots = IteratorOverNodes && std::invocable; -template +template struct ListOfLists { + std::shared_ptr metrics; + private: std::vector> _lists; std::mutex _mutex; - // std::shared_ptr _metrics; // TODO public: // ListOfLists(); auto add(std::shared_ptr list) -> void { auto guard = std::lock_guard(_mutex); + // make sure that list uses our metrics + list->set_metrics(metrics); + if (metrics) { + metrics->increment_total_lists(); + metrics->increment_existing_lists(); + } // make sure that expired nodes are deleted std::erase_if(_lists, [&](auto const& list) { return list.expired(); }); _lists.emplace_back(list); @@ -67,7 +75,11 @@ struct ListOfLists { } } } - // auto set_metrics(std::shared_ptr metrics) -> void; // TODO + + auto set_metrics(std::shared_ptr new_metrics) -> void { + auto guard = std::lock_guard(_mutex); + metrics = new_metrics; + } // void run_external_cleanup() noexcept; }; diff --git a/lib/Containers/Concurrent/ThreadOwnedList.h b/lib/Containers/Concurrent/ThreadOwnedList.h index 5b6c2e8c5627..332d1b0f37fb 100644 --- a/lib/Containers/Concurrent/ThreadOwnedList.h +++ b/lib/Containers/Concurrent/ThreadOwnedList.h @@ -24,6 +24,7 @@ #include "Containers/Concurrent/thread.h" #include "Containers/Concurrent/snapshot.h" +#include "Containers/Concurrent/metrics.h" #include "Inspection/Format.h" #include "fmt/core.h" @@ -32,15 +33,6 @@ namespace arangodb::containers { -template -concept DefinesMetrics = requires(M m) { - m.increment_total_nodes(); - m.increment_registered_nodes(); - m.decrement_registered_nodes(); - m.increment_ready_for_deletion_nodes(); - m.decrement_ready_for_deletion_nodes(); -}; - /** This list is owned by one thread: nodes can only be added on this thread but other threads can read the list and mark nodes for deletion. @@ -50,9 +42,9 @@ concept DefinesMetrics = requires(M m) { shared_ptr to this list. Garbage collection can run either on the owning thread or on another thread (there are two distinct functions for gc). */ -template // T requires a Snapshot type +template struct ThreadOwnedList - : public std::enable_shared_from_this> { + : public std::enable_shared_from_this> { basics::ThreadId const thread; struct Node { @@ -61,7 +53,7 @@ struct ThreadOwnedList std::atomic previous = nullptr; // needs to be atomic for gc from different thread Node* next_to_free = nullptr; - std::shared_ptr> + std::shared_ptr> list; // to be able to mark for deletion }; @@ -70,17 +62,25 @@ struct ThreadOwnedList std::atomic _free_head = nullptr; std::mutex _mutex; // gc and reading cannot happen at same time (perhaps I // can get rid of this with epoch based reclamation) - M metrics; + std::shared_ptr _metrics; public: - static auto make() noexcept -> std::shared_ptr { + static auto make( + std::shared_ptr metrics = std::make_shared()) noexcept + -> std::shared_ptr { struct MakeShared : ThreadOwnedList { - MakeShared() : ThreadOwnedList() {} + MakeShared(std::shared_ptr shared_metrics) + : ThreadOwnedList(shared_metrics) {} }; - return std::make_shared(); + return std::make_shared(metrics); } - ~ThreadOwnedList() noexcept { cleanup(); } + ~ThreadOwnedList() noexcept { + if (_metrics) { + _metrics->decrement_existing_lists(); + } + cleanup(); + } template requires std::invocable @@ -110,8 +110,10 @@ struct ThreadOwnedList } // (1) - this store synchronizes with load in (2) _head.store(node, std::memory_order_release); - metrics.increment_registered_nodes(); - metrics.increment_total_nodes(); + if (_metrics) { + _metrics->increment_registered_nodes(); + _metrics->increment_total_nodes(); + } return node; } @@ -136,8 +138,10 @@ struct ThreadOwnedList // DO NOT access promise after this line. The owner thread might already // be running a cleanup and promise might be deleted. - metrics.decrement_registered_nodes(); - metrics.increment_ready_for_deletion_nodes(); + if (_metrics) { + _metrics->decrement_registered_nodes(); + _metrics->increment_ready_for_deletion_nodes(); + } // self destroyed here. registry might be destroyed here as well. } @@ -172,7 +176,9 @@ struct ThreadOwnedList current = next; next = next->next_to_free; if (current->previous != nullptr) { // TODO memory order - metrics.decrement_ready_for_deletion_nodes(); + if (_metrics) { + _metrics->decrement_ready_for_deletion_nodes(); + } remove(current); delete current; } else { @@ -198,8 +204,19 @@ struct ThreadOwnedList } } + auto set_metrics(std::shared_ptr metrics) -> void { + _metrics = metrics; + } + private: - ThreadOwnedList() noexcept : thread{basics::ThreadId::current()} {} + ThreadOwnedList(std::shared_ptr metrics) noexcept + : thread{basics::ThreadId::current()}, _metrics{metrics} { + // is now done in ListOfLists + // if (_metrics) { + // _metrics->increment_total_lists(); + // _metrics->increment_existing_lists(); + // } + } auto cleanup() noexcept -> void { // (5) - this exchange synchronizes with compare_exchange_weak in (4) @@ -208,7 +225,9 @@ struct ThreadOwnedList while (next != nullptr) { current = next; next = next->next_to_free; - metrics.decrement_ready_for_deletion_nodes(); + if (_metrics) { + _metrics->decrement_ready_for_deletion_nodes(); + } remove(current); delete current; } diff --git a/lib/Containers/Concurrent/metrics.h b/lib/Containers/Concurrent/metrics.h new file mode 100644 index 000000000000..ffe446974701 --- /dev/null +++ b/lib/Containers/Concurrent/metrics.h @@ -0,0 +1,46 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#pragma once + +#include + +namespace arangodb::containers { + +struct Metrics { + virtual ~Metrics() = default; + virtual auto increment_total_nodes() -> void {} + virtual auto increment_registered_nodes() -> void {} + virtual auto decrement_registered_nodes() -> void {} + virtual auto increment_ready_for_deletion_nodes() -> void {} + virtual auto decrement_ready_for_deletion_nodes() -> void {} + virtual auto increment_total_lists() -> void {} + virtual auto increment_existing_lists() -> void {} + virtual auto decrement_existing_lists() -> void {} +}; + +template +concept UpdatesMetrics = requires(T t, std::shared_ptr m) { + t.set_metrics(m); +}; + +} // namespace arangodb::containers diff --git a/tests/Containers/Concurrent/ListOfListsTest.cpp b/tests/Containers/Concurrent/ListOfListsTest.cpp index cb77dc2ccea7..80f0d253cb02 100644 --- a/tests/Containers/Concurrent/ListOfListsTest.cpp +++ b/tests/Containers/Concurrent/ListOfListsTest.cpp @@ -39,9 +39,26 @@ struct MyData { auto snapshot() const -> Snapshot { return Snapshot{.number = number}; } }; +struct MyMetrics : Metrics { + size_t lists = 0; + auto increment_existing_lists() -> void override { lists++; } + auto decrement_existing_lists() -> void override { lists--; } +}; + struct MyNodeList { std::vector data; + std::shared_ptr metrics; + MyNodeList(std::vector data) : data{std::move(data)} { + if (metrics) { + metrics->increment_existing_lists(); + } + } + ~MyNodeList() { + if (metrics) { + metrics->decrement_existing_lists(); + } + } template requires std::invocable auto for_node(F&& function) -> void { @@ -49,6 +66,10 @@ struct MyNodeList { function(item.snapshot()); } } + + auto set_metrics(std::shared_ptr new_metrics) -> void { + metrics = new_metrics; + } }; using MyList = ListOfLists; @@ -90,3 +111,27 @@ TEST(ListOfListsTest, iterates_over_list_items) { EXPECT_EQ(nodes_in_list(list), (std::vector{{1}, {2}, {3}, {4}, {5}, {6}})); } + +TEST(ListOfListsTest, uses_list_of_lists_metrics_for_all_lists) { + MyList list; + auto inner_list = std::make_shared(std::vector{1, 3, 4}); + list.add(inner_list); + + EXPECT_EQ(dynamic_cast(list.metrics.get()), + nullptr); // uses default empty metrics + + auto newMetrics = std::make_shared(); + list.set_metrics(newMetrics); + EXPECT_NE(dynamic_cast(list.metrics.get()), nullptr); + + auto first_inner_list = + std::make_shared(std::vector{1, 2, 3}); + auto second_inner_list = + std::make_shared(std::vector{4, 5, 6}); + list.add(first_inner_list); + list.add(second_inner_list); + + EXPECT_NE(dynamic_cast(list.metrics.get()), nullptr); + EXPECT_EQ(newMetrics->lists, 2); + EXPECT_EQ(dynamic_cast(list.metrics.get())->lists, 2); +} diff --git a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp index 84ceefb4bcc6..fc9e5f652103 100644 --- a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp +++ b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp @@ -59,15 +59,8 @@ struct NodeData : InstanceCounterValue { }; auto snapshot() -> Snapshot { return Snapshot{.number = number}; } }; -struct Metrics { - auto increment_total_nodes() -> void{}; - auto increment_registered_nodes() -> void{}; - auto decrement_registered_nodes() -> void{}; - auto increment_ready_for_deletion_nodes() -> void{}; - auto decrement_ready_for_deletion_nodes() -> void{}; -}; -using MyList = ThreadOwnedList; +using MyList = ThreadOwnedList; auto nodes_in_registry(std::shared_ptr registry) -> std::vector { From d2643db94435b64b8cc9f9da0702b483e38660dc Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Mon, 31 Mar 2025 14:54:37 +0200 Subject: [PATCH 04/11] Add external gc --- ...ListOfThreadOwnedLists.h => ListOfLists.h} | 22 ++++++++++++++++++- .../Containers/Concurrent/ListOfListsTest.cpp | 20 ++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) rename lib/Containers/Concurrent/{ListOfThreadOwnedLists.h => ListOfLists.h} (84%) diff --git a/lib/Containers/Concurrent/ListOfThreadOwnedLists.h b/lib/Containers/Concurrent/ListOfLists.h similarity index 84% rename from lib/Containers/Concurrent/ListOfThreadOwnedLists.h rename to lib/Containers/Concurrent/ListOfLists.h index d54ee9e423ca..0fce163a0f6f 100644 --- a/lib/Containers/Concurrent/ListOfThreadOwnedLists.h +++ b/lib/Containers/Concurrent/ListOfLists.h @@ -37,8 +37,13 @@ concept IteratorOverNodes = requires(List l, F f) { template concept IteratorOverSnapshots = IteratorOverNodes && std::invocable; +template +concept HasExernalGarbageCollection = requires(T t) { + t.garbage_collect_external(); +}; template +requires HasExernalGarbageCollection struct ListOfLists { std::shared_ptr metrics; @@ -80,7 +85,22 @@ struct ListOfLists { auto guard = std::lock_guard(_mutex); metrics = new_metrics; } - // void run_external_cleanup() noexcept; + + /** + Runs an external clean up. + */ + void run_external_cleanup() noexcept { + auto lists = [&] { + auto guard = std::lock_guard(_mutex); + return _lists; + }(); + + for (auto& weak_list : lists) { + if (auto list = weak_list.lock()) { + list->garbage_collect_external(); + } + } + } }; } // namespace arangodb::containers diff --git a/tests/Containers/Concurrent/ListOfListsTest.cpp b/tests/Containers/Concurrent/ListOfListsTest.cpp index 80f0d253cb02..0695df3280ac 100644 --- a/tests/Containers/Concurrent/ListOfListsTest.cpp +++ b/tests/Containers/Concurrent/ListOfListsTest.cpp @@ -20,7 +20,7 @@ /// /// @author Julia Volmer //////////////////////////////////////////////////////////////////////////////// -#include "Containers/Concurrent/ListOfThreadOwnedLists.h" +#include "Containers/Concurrent/ListOfLists.h" #include #include @@ -48,6 +48,7 @@ struct MyMetrics : Metrics { struct MyNodeList { std::vector data; std::shared_ptr metrics; + bool isGarbageCollected = false; MyNodeList(std::vector data) : data{std::move(data)} { if (metrics) { @@ -70,6 +71,8 @@ struct MyNodeList { auto set_metrics(std::shared_ptr new_metrics) -> void { metrics = new_metrics; } + + auto garbage_collect_external() -> void { isGarbageCollected = true; } }; using MyList = ListOfLists; @@ -135,3 +138,18 @@ TEST(ListOfListsTest, uses_list_of_lists_metrics_for_all_lists) { EXPECT_EQ(newMetrics->lists, 2); EXPECT_EQ(dynamic_cast(list.metrics.get())->lists, 2); } + +TEST(ListOfListsTest, executes_garbage_collection_on_each_list) { + MyList list; + auto first_inner_list = + std::make_shared(std::vector{1, 2, 3}); + auto second_inner_list = + std::make_shared(std::vector{4, 5, 6}); + list.add(first_inner_list); + list.add(second_inner_list); + + list.run_external_cleanup(); + + EXPECT_TRUE(first_inner_list->isGarbageCollected); + EXPECT_TRUE(second_inner_list->isGarbageCollected); +} From d6e66e4644452f8a91526b84da2aa961edb42e13 Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Mon, 31 Mar 2025 16:13:41 +0200 Subject: [PATCH 05/11] Make Item an associated type of ThreadOwnedList --- lib/Containers/Concurrent/ListOfLists.h | 18 +++++++++++------- lib/Containers/Concurrent/ThreadOwnedList.h | 1 + .../Containers/Concurrent/ListOfListsTest.cpp | 5 +++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/lib/Containers/Concurrent/ListOfLists.h b/lib/Containers/Concurrent/ListOfLists.h index 0fce163a0f6f..2347875911c7 100644 --- a/lib/Containers/Concurrent/ListOfLists.h +++ b/lib/Containers/Concurrent/ListOfLists.h @@ -30,20 +30,25 @@ namespace arangodb::containers { +template +concept HasItemType = requires(List l) { + typename List::Item; +}; template concept IteratorOverNodes = requires(List l, F f) { l.for_node(f); }; -template -concept IteratorOverSnapshots = - IteratorOverNodes && std::invocable; +template +concept IteratorOverSnapshots = IteratorOverNodes && + std::invocable; template concept HasExernalGarbageCollection = requires(T t) { t.garbage_collect_external(); }; -template -requires HasExernalGarbageCollection +template +requires HasItemType && UpdatesMetrics && + HasExernalGarbageCollection struct ListOfLists { std::shared_ptr metrics; @@ -52,7 +57,6 @@ struct ListOfLists { std::mutex _mutex; public: - // ListOfLists(); auto add(std::shared_ptr list) -> void { auto guard = std::lock_guard(_mutex); // make sure that list uses our metrics @@ -67,7 +71,7 @@ struct ListOfLists { } template - requires IteratorOverSnapshots + requires IteratorOverSnapshots auto for_node(F&& function) -> void { auto lists = [&] { auto guard = std::lock_guard(_mutex); diff --git a/lib/Containers/Concurrent/ThreadOwnedList.h b/lib/Containers/Concurrent/ThreadOwnedList.h index 332d1b0f37fb..3360f695c935 100644 --- a/lib/Containers/Concurrent/ThreadOwnedList.h +++ b/lib/Containers/Concurrent/ThreadOwnedList.h @@ -45,6 +45,7 @@ namespace arangodb::containers { template struct ThreadOwnedList : public std::enable_shared_from_this> { + using Item = T; basics::ThreadId const thread; struct Node { diff --git a/tests/Containers/Concurrent/ListOfListsTest.cpp b/tests/Containers/Concurrent/ListOfListsTest.cpp index 0695df3280ac..4b26a146cac7 100644 --- a/tests/Containers/Concurrent/ListOfListsTest.cpp +++ b/tests/Containers/Concurrent/ListOfListsTest.cpp @@ -46,6 +46,7 @@ struct MyMetrics : Metrics { }; struct MyNodeList { + using Item = MyData; std::vector data; std::shared_ptr metrics; bool isGarbageCollected = false; @@ -75,7 +76,7 @@ struct MyNodeList { auto garbage_collect_external() -> void { isGarbageCollected = true; } }; -using MyList = ListOfLists; +using MyList = ListOfLists; auto nodes_in_list(MyList& registry) -> std::vector { std::vector nodes; @@ -121,7 +122,7 @@ TEST(ListOfListsTest, uses_list_of_lists_metrics_for_all_lists) { list.add(inner_list); EXPECT_EQ(dynamic_cast(list.metrics.get()), - nullptr); // uses default empty metrics + nullptr); // uses default empty auto newMetrics = std::make_shared(); list.set_metrics(newMetrics); From eca1c885ffc1706e1c50baf96aa24d93b1a0c9ce Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Mon, 31 Mar 2025 16:34:47 +0200 Subject: [PATCH 06/11] Make thread list able to set node to 'delete' --- lib/Containers/Concurrent/ThreadOwnedList.h | 25 +++++++++++++------ .../Concurrent/ThreadOwnedListTest.cpp | 4 +++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/lib/Containers/Concurrent/ThreadOwnedList.h b/lib/Containers/Concurrent/ThreadOwnedList.h index 3360f695c935..01069b89a768 100644 --- a/lib/Containers/Concurrent/ThreadOwnedList.h +++ b/lib/Containers/Concurrent/ThreadOwnedList.h @@ -33,6 +33,10 @@ namespace arangodb::containers { +template +concept CanBeSetToDeleted = requires(T t) { + t.set_to_deleted(); +}; /** This list is owned by one thread: nodes can only be added on this thread but other threads can read the list and mark nodes for deletion. @@ -42,7 +46,8 @@ namespace arangodb::containers { shared_ptr to this list. Garbage collection can run either on the owning thread or on another thread (there are two distinct functions for gc). */ -template +template +requires HasSnapshot && CanBeSetToDeleted struct ThreadOwnedList : public std::enable_shared_from_this> { using Item = T; @@ -51,11 +56,18 @@ struct ThreadOwnedList struct Node { T data; Node* next = nullptr; - std::atomic previous = - nullptr; // needs to be atomic for gc from different thread + // this needs to be an atomic because it is accessed during garbage + // collection which can happen in a different thread. This thread will + // load the value. Since there is only one transition, i.e. from nullptr + // to non-null ptr, any missed update will result in a pessimistic + // execution and not an error. More precise, the item might not be + // deleted, although it is not in head position and can be deleted. It + // will be deleted next round. + std::atomic previous = nullptr; Node* next_to_free = nullptr; - std::shared_ptr> - list; // to be able to mark for deletion + // identifies the promise list it belongs to, to be able to mark for + // deletion + std::shared_ptr> list; }; private: @@ -122,8 +134,7 @@ struct ThreadOwnedList // makes sure that promise is really in this list ADB_PROD_ASSERT(node->list.get() == this); - // TODO needs to be done in Promise::mark_for_deletion instead - // promise->state.store(State::Deleted); + node->data.set_to_deleted(); // keep a local copy of the shared pointer. This promise might be the // last of the registry. diff --git a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp index fc9e5f652103..f9beb6cd9ea2 100644 --- a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp +++ b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp @@ -50,6 +50,7 @@ std::size_t InstanceCounterValue::instanceCounter = 0; struct NodeData : InstanceCounterValue { int number; + bool isDeleted = false; NodeData(int number) : number{number} {} @@ -58,6 +59,7 @@ struct NodeData : InstanceCounterValue { bool operator==(Snapshot const&) const = default; }; auto snapshot() -> Snapshot { return Snapshot{.number = number}; } + auto set_to_deleted() -> void { isDeleted = true; }; }; using MyList = ThreadOwnedList; @@ -145,6 +147,8 @@ TEST_F(ThreadOwnedListTest, marked_promises_are_deleted_in_garbage_collection) { EXPECT_EQ(nodes_in_registry(registry), (std::vector{another_node->data.snapshot(), node_to_delete->data.snapshot()})); + EXPECT_TRUE(node_to_delete->data.isDeleted); + EXPECT_FALSE(another_node->data.isDeleted); registry->garbage_collect(); EXPECT_EQ(nodes_in_registry(registry), From b33dae7c5732eb8db0eff65e14ee6bb275a402d1 Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Tue, 1 Apr 2025 14:36:34 +0200 Subject: [PATCH 07/11] Use new, more general types for async registry --- arangod/AsyncRegistryServer/Feature.cpp | 33 +++- arangod/AsyncRegistryServer/Feature.h | 40 +++- arangod/AsyncRegistryServer/RestHandler.cpp | 4 +- lib/Async/Registry/CMakeLists.txt | 5 +- lib/Async/Registry/promise.cpp | 55 ++---- lib/Async/Registry/promise.h | 69 +++---- lib/Async/Registry/registry_variable.cpp | 15 +- lib/Async/Registry/registry_variable.h | 9 +- lib/Async/include/Async/async.h | 3 +- lib/Containers/Concurrent/ThreadOwnedList.cpp | 179 ------------------ lib/Containers/Concurrent/ThreadOwnedList.h | 5 +- lib/Containers/Concurrent/thread.h | 5 + tests/Async/AsyncTest.cpp | 154 ++++++++------- tests/Async/AsyncTestLineNumbers.tpp | 10 +- tests/Async/Registry/CMakeLists.txt | 3 +- tests/Async/Registry/RegistryTest.cpp | 171 +++++++---------- .../Concurrent/ThreadOwnedListTest.cpp | 54 +++--- tests/Futures/FutureCoroutineTest.cpp | 18 +- tests/Futures/FutureTest.cpp | 14 +- 19 files changed, 332 insertions(+), 514 deletions(-) delete mode 100644 lib/Containers/Concurrent/ThreadOwnedList.cpp diff --git a/arangod/AsyncRegistryServer/Feature.cpp b/arangod/AsyncRegistryServer/Feature.cpp index 28f982fdeab2..a943a6a5550e 100644 --- a/arangod/AsyncRegistryServer/Feature.cpp +++ b/arangod/AsyncRegistryServer/Feature.cpp @@ -49,6 +49,31 @@ DECLARE_GAUGE( arangodb_async_existing_thread_registries, std::uint64_t, "Number of threads that started currently existing asyncronous operations"); +auto Feature::RegistryMetrics::increment_total_nodes() -> void { + promises_total->count(); +} +auto Feature::RegistryMetrics::increment_registered_nodes() -> void { + existing_promises->fetch_add(1); +} +auto Feature::RegistryMetrics::decrement_registered_nodes() -> void { + existing_promises->fetch_sub(1); +} +auto Feature::RegistryMetrics::increment_ready_for_deletion_nodes() -> void { + existing_promises->fetch_add(1); +} +auto Feature::RegistryMetrics::decrement_ready_for_deletion_nodes() -> void { + existing_promises->fetch_sub(1); +} +auto Feature::RegistryMetrics::increment_total_lists() -> void { + thread_registries_total->count(); +} +auto Feature::RegistryMetrics::increment_existing_lists() -> void { + existing_thread_registries->fetch_add(1); +} +auto Feature::RegistryMetrics::decrement_existing_lists() -> void { + existing_thread_registries->fetch_sub(1); +} + Feature::Feature(Server& server) : ArangodFeature{server, *this}, _async_mutex{_schedulerWrapper} { startsAfter(); @@ -56,8 +81,8 @@ Feature::Feature(Server& server) } auto Feature::create_metrics(arangodb::metrics::MetricsFeature& metrics_feature) - -> std::shared_ptr { - return std::make_shared( + -> std::shared_ptr { + return std::make_shared( metrics_feature.addShared(arangodb_async_promises_total{}), metrics_feature.addShared(arangodb_async_existing_promises{}), metrics_feature.addShared(arangodb_async_ready_for_deletion_promises{}), @@ -114,4 +139,6 @@ void Feature::collectOptions(std::shared_ptr options) { R"(Each thread that is involved in the async-registry needs to garbage collect its finished async function calls regularly. This option controls how often this is done in seconds. This can possibly be performance relevant because each involved thread aquires a lock.)"); } -Feature::~Feature() { registry.set_metrics(std::make_shared()); } +Feature::~Feature() { + registry.set_metrics(std::make_shared()); +} diff --git a/arangod/AsyncRegistryServer/Feature.h b/arangod/AsyncRegistryServer/Feature.h index 2ba2ca86cd77..6fb37f4fa1b5 100644 --- a/arangod/AsyncRegistryServer/Feature.h +++ b/arangod/AsyncRegistryServer/Feature.h @@ -23,7 +23,7 @@ #pragma once #include "Async/Registry/registry_variable.h" -#include "Async/Registry/Metrics.h" +#include "Metrics/Fwd.h" #include "Basics/FutureSharedLock.h" #include "RestServer/arangod.h" #include "Scheduler/SchedulerFeature.h" @@ -32,8 +32,42 @@ namespace arangodb::async_registry { class Feature final : public ArangodFeature { private: + struct RegistryMetrics : arangodb::containers::Metrics { + RegistryMetrics( + std::shared_ptr promises_total, + std::shared_ptr> existing_promises, + std::shared_ptr> + ready_for_deletion_promises, + std::shared_ptr thread_registries_total, + std::shared_ptr> + existing_thread_registries) + : promises_total{promises_total}, + existing_promises{existing_promises}, + ready_for_deletion_promises{ready_for_deletion_promises}, + thread_registries_total{thread_registries_total}, + existing_thread_registries{existing_thread_registries} {} + ~RegistryMetrics() = default; + auto increment_total_nodes() -> void override; + auto increment_registered_nodes() -> void override; + auto decrement_registered_nodes() -> void override; + auto increment_ready_for_deletion_nodes() -> void override; + auto decrement_ready_for_deletion_nodes() -> void override; + auto increment_total_lists() -> void override; + auto increment_existing_lists() -> void override; + auto decrement_existing_lists() -> void override; + + private: + std::shared_ptr promises_total = nullptr; + std::shared_ptr> existing_promises = nullptr; + std::shared_ptr> ready_for_deletion_promises = + nullptr; + std::shared_ptr thread_registries_total = nullptr; + std::shared_ptr> existing_thread_registries = + nullptr; + }; + static auto create_metrics(arangodb::metrics::MetricsFeature& metrics_feature) - -> std::shared_ptr; + -> std::shared_ptr; struct SchedulerWrapper { using WorkHandle = Scheduler::WorkHandle; template @@ -68,7 +102,7 @@ class Feature final : public ArangodFeature { }; Options _options; - std::shared_ptr metrics; + std::shared_ptr metrics; struct PromiseCleanupThread; std::shared_ptr _cleanupThread; diff --git a/arangod/AsyncRegistryServer/RestHandler.cpp b/arangod/AsyncRegistryServer/RestHandler.cpp index 155884281fcc..ab40713a3959 100644 --- a/arangod/AsyncRegistryServer/RestHandler.cpp +++ b/arangod/AsyncRegistryServer/RestHandler.cpp @@ -68,13 +68,13 @@ namespace { auto all_undeleted_promises() -> ForestWithRoots { Forest forest; std::vector roots; - registry.for_promise([&](PromiseSnapshot promise) { + registry.for_node([&](PromiseSnapshot promise) { if (promise.state != State::Deleted) { std::visit(overloaded{ [&](PromiseId async_waiter) { forest.insert(promise.id, async_waiter, promise); }, - [&](ThreadId sync_waiter_thread) { + [&](basics::ThreadId sync_waiter_thread) { forest.insert(promise.id, nullptr, promise); roots.emplace_back(promise.id); }, diff --git a/lib/Async/Registry/CMakeLists.txt b/lib/Async/Registry/CMakeLists.txt index d8d774c1a3fa..1c3f779f3b35 100644 --- a/lib/Async/Registry/CMakeLists.txt +++ b/lib/Async/Registry/CMakeLists.txt @@ -1,12 +1,11 @@ add_library(arango_async_registry STATIC promise.cpp - registry.cpp - registry_variable.cpp - thread_registry.cpp) + registry_variable.cpp) target_include_directories(arango_async_registry PRIVATE ${PROJECT_SOURCE_DIR}/arangod) target_link_libraries(arango_async_registry PRIVATE + arango_thread_owning_list arango_assertions PUBLIC arango_metrics_base) diff --git a/lib/Async/Registry/promise.cpp b/lib/Async/Registry/promise.cpp index 5f6180f6d119..6b30cba68174 100644 --- a/lib/Async/Registry/promise.cpp +++ b/lib/Async/Registry/promise.cpp @@ -21,67 +21,50 @@ /// @author Julia Volmer //////////////////////////////////////////////////////////////////////////////// #include "promise.h" -#include +#include "Containers/Concurrent/ThreadOwnedList.h" #include "Async/Registry/registry_variable.h" -#include "Async/Registry/thread_registry.h" -#include "Basics/Thread.h" -#include "Inspection/Format.h" +#include "Containers/Concurrent/thread.h" using namespace arangodb::async_registry; -auto ThreadId::current() noexcept -> ThreadId { - return ThreadId{.posix_id = arangodb::Thread::currentThreadId(), - .kernel_id = arangodb::Thread::currentKernelThreadId()}; -} -auto ThreadId::name() -> std::string { - return std::string{ThreadNameFetcher{posix_id}.get()}; -} - -auto Requester::current_thread() -> Requester { return {ThreadId::current()}; } - -Promise::Promise(Promise* next, std::shared_ptr registry, - Requester requester, std::source_location entry_point) - : thread{registry->thread}, +Promise::Promise(Requester requester, std::source_location entry_point) + : thread{basics::ThreadId::current()}, source_location{entry_point.file_name(), entry_point.function_name(), entry_point.line()}, - requester{requester}, - registry{std::move(registry)}, - next{next} {} - -auto Promise::mark_for_deletion() noexcept -> void { - registry->mark_for_deletion(this); -} + requester{requester} {} AddToAsyncRegistry::AddToAsyncRegistry(std::source_location loc) - : promise_in_registry{get_thread_registry().add_promise( - *get_current_coroutine(), std::move(loc))} {} + : node_in_registry{get_thread_registry().add([&]() { + return Promise{*get_current_coroutine(), std::move(loc)}; + })} {} + AddToAsyncRegistry::~AddToAsyncRegistry() { - if (promise_in_registry != nullptr) { - promise_in_registry->mark_for_deletion(); + if (node_in_registry != nullptr) { + node_in_registry->list->mark_for_deletion(node_in_registry.get()); } } auto AddToAsyncRegistry::update_requester(Requester new_requester) -> void { - if (promise_in_registry != nullptr) { - promise_in_registry->requester.store(new_requester); + if (node_in_registry != nullptr) { + node_in_registry->data.requester.store(new_requester); } } auto AddToAsyncRegistry::id() -> void* { - if (promise_in_registry != nullptr) { - return promise_in_registry->id(); + if (node_in_registry != nullptr) { + return node_in_registry->data.id(); } else { return nullptr; } } auto AddToAsyncRegistry::update_source_location(std::source_location loc) -> void { - if (promise_in_registry != nullptr) { - promise_in_registry->source_location.line.store(loc.line()); + if (node_in_registry != nullptr) { + node_in_registry->data.source_location.line.store(loc.line()); } } auto AddToAsyncRegistry::update_state(State state) -> std::optional { - if (promise_in_registry != nullptr) { - return promise_in_registry->state.exchange(state); + if (node_in_registry != nullptr) { + return node_in_registry->data.state.exchange(state); } else { return std::nullopt; } diff --git a/lib/Async/Registry/promise.h b/lib/Async/Registry/promise.h index 830c6f8f0ab7..c67987cde58c 100644 --- a/lib/Async/Registry/promise.h +++ b/lib/Async/Registry/promise.h @@ -22,15 +22,15 @@ //////////////////////////////////////////////////////////////////////////////// #pragma once +#include #include #include #include #include #include #include -#include "Basics/threads-posix.h" -#include "Inspection/Format.h" -#include "Inspection/Types.h" +#include "Containers/Concurrent/ThreadOwnedList.h" +#include "Containers/Concurrent/thread.h" #include "fmt/format.h" #include "fmt/std.h" @@ -43,28 +43,19 @@ struct overloaded : Ts... { template overloaded(Ts...) -> overloaded; } // namespace + namespace arangodb::async_registry { struct ThreadRegistry; -struct ThreadId { - static auto current() noexcept -> ThreadId; - auto name() -> std::string; - TRI_tid_t posix_id; - pid_t kernel_id; - bool operator==(ThreadId const&) const = default; -}; -template -auto inspect(Inspector& f, ThreadId& x) { - return f.object(x).fields(f.field("LWPID", x.kernel_id), - f.field("name", x.name())); -} - struct SourceLocationSnapshot { std::string_view file_name; std::string_view function_name; std::uint_least32_t line; bool operator==(SourceLocationSnapshot const&) const = default; + static auto from(std::source_location loc) -> SourceLocationSnapshot { + return SourceLocationSnapshot{.file_name=loc.file_name(), .function_name=loc.function_name(), .line=loc.line()}; + } }; template auto inspect(Inspector& f, SourceLocationSnapshot& x) { @@ -101,7 +92,7 @@ auto inspect(Inspector& f, PromiseIdWrapper& x) { return f.object(x).fields(f.field("promise", fmt::format("{}", x.item))); } struct ThreadIdWrapper { - ThreadId item; + basics::ThreadId item; }; template auto inspect(Inspector& f, ThreadIdWrapper& x) { @@ -114,8 +105,10 @@ auto inspect(Inspector& f, RequesterWrapper& x) { inspection::inlineType(), inspection::inlineType()); } -struct Requester : std::variant { - static auto current_thread() -> Requester; +struct Requester : std::variant { + static auto current_thread() -> Requester { + return {basics::ThreadId::current()}; + } }; template auto inspect(Inspector& f, Requester& x) { @@ -125,7 +118,7 @@ auto inspect(Inspector& f, Requester& x) { [&](PromiseId waiter) { return RequesterWrapper{PromiseIdWrapper{waiter}}; }, - [&](ThreadId waiter) { + [&](basics::ThreadId waiter) { return RequesterWrapper{ThreadIdWrapper{waiter}}; }, }, @@ -136,7 +129,7 @@ auto inspect(Inspector& f, Requester& x) { struct PromiseSnapshot { void* id; - ThreadId thread; + basics::ThreadId thread; SourceLocationSnapshot source_location; Requester requester; State state; @@ -150,39 +143,29 @@ auto inspect(Inspector& f, PromiseSnapshot& x) { f.field("requester", x.requester), f.field("state", x.state)); } + struct Promise { - Promise(Promise* next, std::shared_ptr registry, - Requester requester, std::source_location location); + using Snapshot = PromiseSnapshot; + Promise(Requester requester, std::source_location location); ~Promise() = default; - auto mark_for_deletion() noexcept -> void; auto id() -> void* { return this; } - auto snapshot() -> PromiseSnapshot { + auto snapshot() -> Snapshot { return PromiseSnapshot{.id = id(), .thread = thread, .source_location = source_location.snapshot(), .requester = requester.load(), .state = state.load()}; } + auto set_to_deleted() -> void { + state.store(State::Deleted, std::memory_order_relaxed); + } - ThreadId thread; + basics::ThreadId thread; SourceLocation source_location; std::atomic requester; std::atomic state = State::Running; - // identifies the promise list it belongs to - std::shared_ptr registry; - Promise* next = nullptr; - // this needs to be an atomic because it is accessed during garbage - // collection which can happen in a different thread. This thread will - // load the value. Since there is only one transition, i.e. from nullptr - // to non-null ptr, any missed update will result in a pessimistic - // execution and not an error. More precise, the item might not be - // deleted, although it is not in head position and can be deleted. It - // will be deleted next round. - std::atomic previous = nullptr; - // only needed to garbage collect promises - Promise* next_to_free = nullptr; }; struct AddToAsyncRegistry { @@ -203,13 +186,9 @@ struct AddToAsyncRegistry { struct noop { void operator()(void*) {} }; - - public: - std::unique_ptr promise_in_registry = nullptr; + std::unique_ptr::Node, noop> + node_in_registry = nullptr; }; } // namespace arangodb::async_registry -template<> -struct fmt::formatter - : arangodb::inspection::inspection_formatter {}; diff --git a/lib/Async/Registry/registry_variable.cpp b/lib/Async/Registry/registry_variable.cpp index 07e950633529..01d3283dd834 100644 --- a/lib/Async/Registry/registry_variable.cpp +++ b/lib/Async/Registry/registry_variable.cpp @@ -21,23 +21,28 @@ /// @author Julia Volmer //////////////////////////////////////////////////////////////////////////////// #include "registry_variable.h" -#include + #include "Async/Registry/promise.h" +#include + namespace arangodb::async_registry { -Registry registry; +containers::ListOfLists> registry; /** Gives the thread registry of the current thread. Creates the thread registry when called for the first time. */ -auto get_thread_registry() noexcept -> ThreadRegistry& { +auto get_thread_registry() noexcept -> containers::ThreadOwnedList& { struct ThreadRegistryGuard { - ThreadRegistryGuard() : _registry{registry.add_thread()} {} + ThreadRegistryGuard() + : _registry{containers::ThreadOwnedList::make()} { + registry.add(_registry); + } - std::shared_ptr _registry; + std::shared_ptr> _registry; }; static thread_local auto registry_guard = ThreadRegistryGuard{}; return *registry_guard._registry; diff --git a/lib/Async/Registry/registry_variable.h b/lib/Async/Registry/registry_variable.h index 6259a9f4c40e..e34668712071 100644 --- a/lib/Async/Registry/registry_variable.h +++ b/lib/Async/Registry/registry_variable.h @@ -22,7 +22,9 @@ //////////////////////////////////////////////////////////////////////////////// #pragma once -#include "registry.h" +#include "Async/Registry/promise.h" +#include "Containers/Concurrent/ListOfLists.h" +#include "Containers/Concurrent/ThreadOwnedList.h" namespace arangodb::async_registry { @@ -30,12 +32,13 @@ namespace arangodb::async_registry { /** Global variable that holds all coroutines. */ -extern Registry registry; +extern containers::ListOfLists> registry; /** Get registry of all active coroutine promises on this thread. */ -auto get_thread_registry() noexcept -> ThreadRegistry&; +auto get_thread_registry() noexcept -> containers::ThreadOwnedList&; auto get_current_coroutine() noexcept -> Requester*; + } // namespace arangodb::async_registry diff --git a/lib/Async/include/Async/async.h b/lib/Async/include/Async/async.h index 4976d3c2e474..3b19d5880a4c 100644 --- a/lib/Async/include/Async/async.h +++ b/lib/Async/include/Async/async.h @@ -1,7 +1,6 @@ #pragma once #include "Async/Registry/promise.h" -#include "Async/Registry/registry.h" #include "Async/Registry/registry_variable.h" #include "Async/coro-utils.h" #include "Async/expected.h" @@ -34,7 +33,7 @@ struct async_promise_base : async_registry::AddToAsyncRegistry { } std::suspend_never initial_suspend() noexcept { - promise_in_registry->state.store(async_registry::State::Running); + node_in_registry->data.state.store(async_registry::State::Running); return {}; } auto final_suspend() noexcept { diff --git a/lib/Containers/Concurrent/ThreadOwnedList.cpp b/lib/Containers/Concurrent/ThreadOwnedList.cpp deleted file mode 100644 index db87aa7a36f2..000000000000 --- a/lib/Containers/Concurrent/ThreadOwnedList.cpp +++ /dev/null @@ -1,179 +0,0 @@ -//////////////////////////////////////////////////////////////////////////////// -/// DISCLAIMER -/// -/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany -/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany -/// -/// Licensed under the Business Source License 1.1 (the "License"); -/// you may not use this file except in compliance with the License. -/// You may obtain a copy of the License at -/// -/// https://github.com/arangodb/arangodb/blob/devel/LICENSE -/// -/// Unless required by applicable law or agreed to in writing, software -/// distributed under the License is distributed on an "AS IS" BASIS, -/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -/// See the License for the specific language governing permissions and -/// limitations under the License. -/// -/// Copyright holder is ArangoDB GmbH, Cologne, Germany -/// -/// @author Julia Volmer -//////////////////////////////////////////////////////////////////////////////// -#include "ThreadOwnedList.h" - -#include "fmt/core.h" - -using namespace arangodb::containers; - -template -auto ThreadOwnedList::make() noexcept - -> std::shared_ptr> { - struct MakeShared : ThreadOwnedList { - MakeShared() : ThreadOwnedList() {} - }; - return std::make_shared(); -} - -template -ThreadOwnedList::ThreadOwnedList() noexcept - : thread{basics::ThreadId::current()} {} - -template -ThreadOwnedList::~ThreadOwnedList() noexcept { - cleanup(); -} - -template -auto ThreadOwnedList::add(T data) noexcept -> Node* { - auto current_thread = basics::ThreadId::current(); - ADB_PROD_ASSERT(current_thread == thread) << fmt::format( - "ThreadOwnedList::add_promise was called from thread {} but needs to be " - "called from ThreadOwnedList's owning thread {}. {}", - current_thread, thread, this); - auto current_head = _head.load(std::memory_order_relaxed); - auto node = new Node{.data = std::move(data), - .next = current_head, - .list = this->shared_from_this()}; - if (current_head != nullptr) { - current_head->previous.store(node); // TODO memory order - } - // (1) - this store synchronizes with load in (2) - _head.store(node, std::memory_order_release); - metrics.increment_registered_nodes(); - metrics.increment_total_nodes(); - return node; -} - -template -auto ThreadOwnedList::mark_for_deletion(Node* node) noexcept -> void { - // makes sure that promise is really in this list - ADB_PROD_ASSERT(node->registry.get() == this); - - // TODO needs to be done in Promise::mark_for_deletion instead - // promise->state.store(State::Deleted); - - // keep a local copy of the shared pointer. This promise might be the - // last of the registry. - auto self = std::move(node->registry); - - auto current_head = _free_head.load(std::memory_order_relaxed); - do { - node->next_to_free = current_head; - // (4) - this compare_exchange_weak synchronizes with exchange in (5) - } while (not _free_head.compare_exchange_weak(current_head, node, - std::memory_order_release, - std::memory_order_acquire)); - // DO NOT access promise after this line. The owner thread might already - // be running a cleanup and promise might be deleted. - - metrics.decrement_registered_nodes(); - metrics.increment_ready_for_deletion_nodes(); - - // self destroyed here. registry might be destroyed here as well. -} - -template -auto ThreadOwnedList::garbage_collect() noexcept -> void { - auto current_thread = basics::ThreadId::current(); - ADB_PROD_ASSERT(current_thread == thread) << fmt::format( - "ThreadOwnedList::garbage_collect was called from thread {} but needs to " - "be called from ThreadOwnedList's owning thread {}. {}", - current_thread, thread, this); - auto guard = std::lock_guard(_mutex); - cleanup(); -} - -template -auto ThreadOwnedList::garbage_collect_external() noexcept -> void { - // acquire the lock. This prevents the owning thread and the observer - // from accessing promises. Note that the owing thread only adds new - // promises to the head of the list. - auto guard = std::lock_guard(_mutex); - // we can make the following observation. Once a promise is enqueued in the - // list, it previous and next pointer is never updated, except for the current - // head element. Also, Promises are only removed, after the mutex has been - // acquired. This implies that we can clean up all promises, that are not - // in head position right now. - Node* maybe_head_ptr = nullptr; - Node*current, - *next = _free_head.exchange( - nullptr, std::memory_order_acquire); // TODO check memory order - while (next != nullptr) { - current = next; - next = next->next_to_free; - if (current->previous != nullptr) { // TODO memory order - metrics.decrement_ready_for_deletion_nodes(); - remove(current); - delete current; - } else { - // if this is the head of the promise list, we cannot delete it because - // additional promises could have been added in the meantime - // (if these new promises would have been marked in the meantime, they - // would be in the new free list due to the exchange earlier) - ADB_PROD_ASSERT(maybe_head_ptr == nullptr); - maybe_head_ptr = current; - } - } - // After the clean up we have to add the potential head back into the free - // list. - if (maybe_head_ptr) { - auto current_head = _free_head.load(std::memory_order_relaxed); - do { - maybe_head_ptr->next_to_free = current_head; - // (4) - this compare_exchange_weak synchronizes with exchange in (5) - // TODO check memory order - } while (not _free_head.compare_exchange_weak(current_head, maybe_head_ptr, - std::memory_order_release, - std::memory_order_acquire)); - } -} - -template -auto ThreadOwnedList::cleanup() noexcept -> void { - // (5) - this exchange synchronizes with compare_exchange_weak in (4) - Node*current, - *next = _free_head.exchange(nullptr, std::memory_order_acquire); - while (next != nullptr) { - current = next; - next = next->next_to_free; - metrics.decrement_ready_for_deletion_nodes(); - remove(current); - delete current; - } -} - -template -auto ThreadOwnedList::remove(Node* node) -> void { - auto* next = node->next; - auto* previous = node->previous.load(); // TODO memory order - if (previous == nullptr) { // promise is current head - // (3) - this store synchronizes with the load in (2) - _head.store(next, std::memory_order_release); - } else { - previous->next = next; - } - if (next != nullptr) { - next->previous = previous; // TODO memory order - } -} diff --git a/lib/Containers/Concurrent/ThreadOwnedList.h b/lib/Containers/Concurrent/ThreadOwnedList.h index 01069b89a768..5af01df29339 100644 --- a/lib/Containers/Concurrent/ThreadOwnedList.h +++ b/lib/Containers/Concurrent/ThreadOwnedList.h @@ -106,7 +106,8 @@ struct ThreadOwnedList } } - auto add(T data) noexcept -> Node* { + template + auto add(F&& create_data) noexcept -> Node* { auto current_thread = basics::ThreadId::current(); ADB_PROD_ASSERT(current_thread == thread) << fmt::format( "ThreadOwnedList::add_promise was called from thread {} but needs to " @@ -115,7 +116,7 @@ struct ThreadOwnedList inspection::json(current_thread), inspection::json(thread), (void*)this); auto current_head = _head.load(std::memory_order_relaxed); - auto node = new Node{.data = std::move(data), + auto node = new Node{.data = create_data(), .next = current_head, .list = this->shared_from_this()}; if (current_head != nullptr) { diff --git a/lib/Containers/Concurrent/thread.h b/lib/Containers/Concurrent/thread.h index 2a2231d08efc..45a7ad290444 100644 --- a/lib/Containers/Concurrent/thread.h +++ b/lib/Containers/Concurrent/thread.h @@ -23,6 +23,7 @@ #pragma once #include "Basics/threads-posix.h" +#include "Inspection/Format.h" namespace arangodb::basics { @@ -40,3 +41,7 @@ auto inspect(Inspector& f, ThreadId& x) { } } // namespace arangodb::basics + +template<> +struct fmt::formatter + : arangodb::inspection::inspection_formatter {}; diff --git a/tests/Async/AsyncTest.cpp b/tests/Async/AsyncTest.cpp index 5a94b509088d..c26327efed8f 100644 --- a/tests/Async/AsyncTest.cpp +++ b/tests/Async/AsyncTest.cpp @@ -1,8 +1,6 @@ #include "Async/async.h" #include "Async/Registry/promise.h" #include "Async/Registry/registry_variable.h" -#include "Async/Registry/registry.h" -#include "Async/Registry/thread_registry.h" #include "Inspection/Format.h" #include "Inspection/JsonPrintInspector.h" #include "Utils/ExecContext.h" @@ -15,11 +13,12 @@ namespace { -auto promise_count(arangodb::async_registry::ThreadRegistry& registry) -> uint { +auto promise_count_in_registry() -> uint { uint promise_count = 0; - registry.for_promise([&](arangodb::async_registry::PromiseSnapshot promise) { - promise_count++; - }); + arangodb::async_registry::get_thread_registry().for_node( + [&](arangodb::async_registry::PromiseSnapshot promise) { + promise_count++; + }); return promise_count; } @@ -153,9 +152,8 @@ struct AsyncTest> : ::testing::Test { arangodb::async_registry::get_thread_registry().garbage_collect(); wait.stop(); EXPECT_EQ(InstanceCounterValue::instanceCounter, 0); - EXPECT_EQ(promise_count(arangodb::async_registry::get_thread_registry()), - 0); - EXPECT_TRUE(std::holds_alternative( + EXPECT_EQ(promise_count_in_registry(), 0); + EXPECT_TRUE(std::holds_alternative( *arangodb::async_registry::get_current_coroutine())); } @@ -360,57 +358,6 @@ TYPED_TEST(AsyncTest, multiple_suspension_points) { EXPECT_EQ(awaitable.await_resume(), 0); } -TYPED_TEST(AsyncTest, coroutine_is_removed_before_registry_entry) { - using ValueType = TypeParam::second_type; - - auto coro = []() -> async { co_return 12; }; - - { - coro().reset(); - EXPECT_EQ(InstanceCounterValue::instanceCounter, 0); - EXPECT_EQ(promise_count(arangodb::async_registry::get_thread_registry()), - 1); - } - { - std::move(coro()).operator co_await().await_resume(); - EXPECT_EQ(InstanceCounterValue::instanceCounter, 0); - EXPECT_EQ(promise_count(arangodb::async_registry::get_thread_registry()), - 2); - } - { - { coro(); } - - EXPECT_EQ(InstanceCounterValue::instanceCounter, 0); - EXPECT_EQ(promise_count(arangodb::async_registry::get_thread_registry()), - 3); - } -} - -namespace { -auto foo() -> async { co_return; } -auto bar() -> async { co_return; } -auto baz() -> async { co_return; } -} // namespace -TYPED_TEST(AsyncTest, promises_are_registered_in_global_async_registry) { - auto coro_foo = foo(); - EXPECT_EQ(promise_count(arangodb::async_registry::get_thread_registry()), 1); - - std::jthread([&]() { - auto coro_bar = bar(); - auto coro_baz = baz(); - - std::vector names; - arangodb::async_registry::registry.for_promise( - [&](arangodb::async_registry::PromiseSnapshot promise) { - names.push_back(promise.source_location.function_name); - }); - EXPECT_EQ(names.size(), 3); - EXPECT_TRUE(names[0].find("foo") != std::string::npos); - EXPECT_TRUE(names[1].find("baz") != std::string::npos); - EXPECT_TRUE(names[2].find("bar") != std::string::npos); - }).join(); -} - struct ExecContext_Waiting : public arangodb::ExecContext { ExecContext_Waiting() : arangodb::ExecContext(arangodb::ExecContext::ConstructorToken{}, @@ -481,12 +428,60 @@ TYPED_TEST(AsyncTest, execution_context_is_local_to_coroutine) { EXPECT_EQ(ExecContext::current().user(), "End"); } +namespace { +auto foo() -> async { co_return; } +auto bar() -> async { co_return; } +auto baz() -> async { co_return; } +} // namespace +TYPED_TEST(AsyncTest, promises_are_registered_in_global_async_registry) { + auto coro_foo = foo(); + EXPECT_EQ(promise_count_in_registry(), 1); + + std::jthread([&]() { + auto coro_bar = bar(); + auto coro_baz = baz(); + + std::vector names; + arangodb::async_registry::registry.for_node( + [&](arangodb::async_registry::PromiseSnapshot promise) { + names.push_back(promise.source_location.function_name); + }); + EXPECT_EQ(names.size(), 3); + EXPECT_TRUE(names[0].find("foo") != std::string::npos); + EXPECT_TRUE(names[1].find("baz") != std::string::npos); + EXPECT_TRUE(names[2].find("bar") != std::string::npos); + }).join(); +} + +TYPED_TEST(AsyncTest, coroutine_is_deleted_earlier_than_registry_entry) { + using ValueType = TypeParam::second_type; + + auto coro = []() -> async { co_return 12; }; + + { + coro().reset(); + EXPECT_EQ(InstanceCounterValue::instanceCounter, 0); + EXPECT_EQ(promise_count_in_registry(), 1); + } + { + std::move(coro()).operator co_await().await_resume(); + EXPECT_EQ(InstanceCounterValue::instanceCounter, 0); + EXPECT_EQ(promise_count_in_registry(), 2); + } + { + { coro(); } + + EXPECT_EQ(InstanceCounterValue::instanceCounter, 0); + EXPECT_EQ(promise_count_in_registry(), 3); + } +} + namespace { auto find_promise_by_name(std::string_view name) -> std::optional { std::optional requested_promise = std::nullopt; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { if (promise.source_location.function_name.find(name) != std::string::npos) { @@ -534,8 +529,8 @@ TYPED_TEST( static auto waiter_fn(TestType test) -> async { auto waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( - waiter_promise->requester)); + EXPECT_TRUE( + std::holds_alternative(waiter_promise->requester)); auto fn = Functions::awaited_fn(test); @@ -554,8 +549,8 @@ TYPED_TEST( // waiter did not change waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( - waiter_promise->requester)); + EXPECT_TRUE( + std::holds_alternative(waiter_promise->requester)); co_return; }; @@ -594,8 +589,8 @@ TYPED_TEST( static auto waiter_fn(TestType test) -> async { auto waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( - waiter_promise->requester)); + EXPECT_TRUE( + std::holds_alternative(waiter_promise->requester)); auto fn = Functions::awaited_fn(test); auto fn_2 = Functions::awaited_2_fn(); @@ -624,8 +619,8 @@ TYPED_TEST( // waiter did not change waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( - waiter_promise->requester)); + EXPECT_TRUE( + std::holds_alternative(waiter_promise->requester)); co_return; }; @@ -645,8 +640,7 @@ TYPED_TEST(AsyncTest, static auto awaited_fn(TestType test) -> async { auto promise = find_promise_by_name("awaited_fn"); EXPECT_TRUE(promise.has_value()); - EXPECT_TRUE( - std::holds_alternative(promise->requester)); + EXPECT_TRUE(std::holds_alternative(promise->requester)); co_await test->wait; @@ -655,13 +649,13 @@ TYPED_TEST(AsyncTest, static auto waiter_fn(async&& fn) -> async { auto waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( - waiter_promise->requester)); + EXPECT_TRUE( + std::holds_alternative(waiter_promise->requester)); auto awaited_promise = find_promise_by_name("awaited_fn"); EXPECT_TRUE(awaited_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( - waiter_promise->requester)); + EXPECT_TRUE( + std::holds_alternative(waiter_promise->requester)); co_await std::move(fn); @@ -673,8 +667,8 @@ TYPED_TEST(AsyncTest, // waiter did not change waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( - waiter_promise->requester)); + EXPECT_TRUE( + std::holds_alternative(waiter_promise->requester)); co_return; }; @@ -705,8 +699,8 @@ TYPED_TEST( auto waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( - waiter_promise->requester)); + EXPECT_TRUE( + std::holds_alternative(waiter_promise->requester)); auto awaited_promise = find_promise_by_name("awaited_fn"); EXPECT_TRUE(awaited_promise.has_value()); @@ -733,7 +727,7 @@ namespace { auto expect_all_promises_in_state(arangodb::async_registry::State state, uint number_of_promises) { uint count = 0; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { count++; EXPECT_EQ(promise.state, state); diff --git a/tests/Async/AsyncTestLineNumbers.tpp b/tests/Async/AsyncTestLineNumbers.tpp index c1009f40e0ab..64d0b14518cf 100644 --- a/tests/Async/AsyncTestLineNumbers.tpp +++ b/tests/Async/AsyncTestLineNumbers.tpp @@ -35,7 +35,7 @@ TEST(AsyncTest, source_location_in_registry_is_co_await_line) { }(); uint count = 0; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { count++; EXPECT_EQ(promise.source_location.line, 34); @@ -53,7 +53,7 @@ TEST(AsyncTest, source_location_in_registry_is_co_await_line) { }(); uint count = 0; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { count++; EXPECT_EQ(promise.source_location.line, 50); @@ -62,7 +62,7 @@ TEST(AsyncTest, source_location_in_registry_is_co_await_line) { wait.resume(); count = 0; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { count++; EXPECT_EQ(promise.source_location.line, 52); @@ -80,7 +80,7 @@ TEST(AsyncTest, source_location_in_registry_is_co_await_line) { }(); uint count = 0; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { count++; EXPECT_EQ(promise.source_location.line, 77); @@ -89,7 +89,7 @@ TEST(AsyncTest, source_location_in_registry_is_co_await_line) { wait.await(); count = 0; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { count++; EXPECT_EQ(promise.source_location.line, 79); diff --git a/tests/Async/Registry/CMakeLists.txt b/tests/Async/Registry/CMakeLists.txt index 0ea9e9c2fd9a..ecc885612e54 100644 --- a/tests/Async/Registry/CMakeLists.txt +++ b/tests/Async/Registry/CMakeLists.txt @@ -1,4 +1,3 @@ target_sources(arangodbtests PRIVATE - RegistryTest.cpp - ThreadRegistryTest.cpp) + RegistryTest.cpp) diff --git a/tests/Async/Registry/RegistryTest.cpp b/tests/Async/Registry/RegistryTest.cpp index 1f0ef835eb49..b75bde7d366d 100644 --- a/tests/Async/Registry/RegistryTest.cpp +++ b/tests/Async/Registry/RegistryTest.cpp @@ -21,7 +21,7 @@ /// @author Julia Volmer //////////////////////////////////////////////////////////////////////////////// #include "Async/Registry/promise.h" -#include "Async/Registry/registry.h" +#include "Async/Registry/registry_variable.h" #include #include @@ -32,136 +32,103 @@ using namespace arangodb::async_registry; namespace { -auto promises_in_registry(Registry& registry) -> std::vector { +auto promises_in_registry() -> std::vector { std::vector promises; - registry.for_promise( + registry.for_node( [&](PromiseSnapshot promise) { promises.push_back(promise); }); return promises; } -} // namespace - -struct AsyncRegistryTest : ::testing::Test {}; +struct MyPromise : public AddToAsyncRegistry { + SourceLocationSnapshot source_location; + basics::ThreadId thread; + MyPromise(std::source_location loc = std::source_location::current()) + : AddToAsyncRegistry{loc}, + source_location{SourceLocationSnapshot::from(std::move(loc))}, + thread{basics::ThreadId::current()} {} + auto snapshot(State state = State::Running) -> PromiseSnapshot { + return PromiseSnapshot{.id = id(), + .thread = thread, + .source_location = source_location, + .requester = {thread}, + .state = state}; + } +}; -TEST_F(AsyncRegistryTest, registers_promise_on_same_thread) { - Registry registry; - auto thread_registry = registry.add_thread(); +} // namespace - auto* promise = thread_registry->add_promise(Requester::current_thread(), - std::source_location::current()); +struct AsyncRegistryTest : ::testing::Test { + void TearDown() override { + // execute garbage collection on current thread + get_thread_registry().garbage_collect(); + } +}; - EXPECT_EQ(promises_in_registry(registry), - std::vector{promise->snapshot()}); +TEST_F(AsyncRegistryTest, registers_created_promise) { + auto promise = MyPromise{}; - promise->mark_for_deletion(); - thread_registry->garbage_collect(); + EXPECT_EQ(promises_in_registry(), + (std::vector{promise.snapshot()})); } TEST_F(AsyncRegistryTest, registers_promise_on_different_threads) { - Registry registry; - std::thread([&]() { - auto thread_registry = registry.add_thread(); - - auto* promise = thread_registry->add_promise( - Requester::current_thread(), std::source_location::current()); + auto promise = MyPromise{}; - EXPECT_EQ(promises_in_registry(registry), - std::vector{promise->snapshot()}); - - promise->mark_for_deletion(); - thread_registry->garbage_collect(); + EXPECT_EQ(promises_in_registry(), + (std::vector{promise.snapshot()})); + // cleans up by itself }).join(); + + EXPECT_EQ(promises_in_registry(), std::vector{}); } TEST_F(AsyncRegistryTest, iterates_over_promises_on_same_thread_in_reverse_order) { - Registry registry; - auto thread_registry = registry.add_thread(); - auto* first_promise = thread_registry->add_promise( - Requester::current_thread(), std::source_location::current()); - auto* second_promise = thread_registry->add_promise( - Requester::current_thread(), std::source_location::current()); - - EXPECT_EQ(promises_in_registry(registry), - (std::vector{second_promise->snapshot(), - first_promise->snapshot()})); - - first_promise->mark_for_deletion(); - second_promise->mark_for_deletion(); - thread_registry->garbage_collect(); - registry.remove_thread(thread_registry.get()); + auto first_promise = MyPromise{}; + auto second_promise = MyPromise{}; + + EXPECT_EQ(promises_in_registry(), + (std::vector{second_promise.snapshot(), + first_promise.snapshot()})); } TEST_F(AsyncRegistryTest, iterates_over_promises_on_differen_threads) { - Registry registry; - auto thread_registry = registry.add_thread(); - auto* first_promise = thread_registry->add_promise( - Requester::current_thread(), std::source_location::current()); + auto outer_thread_promise = MyPromise{}; std::thread([&]() { - auto thread_registry = registry.add_thread(); - auto* second_promise = thread_registry->add_promise( - Requester::current_thread(), std::source_location::current()); - - EXPECT_EQ(promises_in_registry(registry), - (std::vector{first_promise->snapshot(), - second_promise->snapshot()})); + auto inner_thread_promise = MyPromise{}; - second_promise->mark_for_deletion(); - thread_registry->garbage_collect(); + EXPECT_EQ(promises_in_registry(), + (std::vector{outer_thread_promise.snapshot(), + inner_thread_promise.snapshot()})); }).join(); - first_promise->mark_for_deletion(); - thread_registry->garbage_collect(); -} - -TEST_F(AsyncRegistryTest, - iteration_after_executed_garbage_collection_is_empty) { - Registry registry; - auto thread_registry = registry.add_thread(); - - auto* promise = thread_registry->add_promise(Requester::current_thread(), - std::source_location::current()); - EXPECT_EQ(promises_in_registry(registry), - (std::vector{promise->snapshot()})); - - promise->mark_for_deletion(); - EXPECT_EQ(promises_in_registry(registry), - (std::vector{promise->snapshot()})); - - thread_registry->garbage_collect(); - EXPECT_EQ(promises_in_registry(registry), (std::vector{})); -} - -TEST_F(AsyncRegistryTest, promises_on_removed_thread_are_still_iterated_over) { - Registry registry; - Promise* promise; - { - auto thread_registry = registry.add_thread(); - promise = thread_registry->add_promise(Requester::current_thread(), - std::source_location::current()); - } - EXPECT_EQ(promises_in_registry(registry), - (std::vector{promise->snapshot()})); - - promise->mark_for_deletion(); + EXPECT_EQ(promises_in_registry(), + (std::vector{outer_thread_promise.snapshot()})); } TEST_F( AsyncRegistryTest, - different_thread_can_mark_promise_for_deletion_after_thread_already_ended) { - Registry registry; - auto thread_registry = registry.add_thread(); - Promise* promise; - - std::thread([&]() { - auto thread_registry_on_different_thread = registry.add_thread(); - promise = thread_registry_on_different_thread->add_promise( - Requester::current_thread(), std::source_location::current()); - }).join(); - - promise->mark_for_deletion(); - - EXPECT_EQ(promises_in_registry(registry), (std::vector{})); + marks_deleted_promise_for_deletion_which_is_deleted_in_garbage_collection) { + PromiseSnapshot promise_in_registry; + { + auto promise = MyPromise{}; + auto promises = promises_in_registry(); + EXPECT_EQ(promises, (std::vector{promise.snapshot()})); + promise_in_registry = promises[0]; + + get_thread_registry() + .garbage_collect(); // does not do anything because nothing + // is yet marked for deletion + EXPECT_EQ(promises_in_registry(), + (std::vector{promise.snapshot()})); + } // marks promise for deletion + + promise_in_registry.state = State::Deleted; + EXPECT_EQ(promises_in_registry(), + (std::vector{promise_in_registry})); + + get_thread_registry().garbage_collect(); + EXPECT_EQ(promises_in_registry(), (std::vector{})); } diff --git a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp index f9beb6cd9ea2..8bbff554e792 100644 --- a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp +++ b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp @@ -66,10 +66,10 @@ using MyList = ThreadOwnedList; auto nodes_in_registry(std::shared_ptr registry) -> std::vector { - std::vector promises; + std::vector nodes; registry->for_node( - [&](NodeData::Snapshot promise) { promises.push_back(promise); }); - return promises; + [&](NodeData::Snapshot promise) { nodes.push_back(promise); }); + return nodes; } } // namespace @@ -83,7 +83,7 @@ using ThreadOwnedListDeathTest = ThreadOwnedListTest; TEST_F(ThreadOwnedListTest, adds_a_promise) { auto registry = MyList::make(); - auto node = registry->add(NodeData{2}); + auto node = registry->add([]() { return NodeData{2}; }); EXPECT_EQ(nodes_in_registry(registry), (std::vector{node->data.snapshot()})); @@ -96,16 +96,18 @@ TEST_F(ThreadOwnedListDeathTest, another_thread_cannot_add_a_promise) { GTEST_FLAG_SET(death_test_style, "threadsafe"); auto registry = MyList::make(); - std::jthread( - [&]() { EXPECT_DEATH(registry->add(NodeData{1}), "Assertion failed"); }); + std::jthread([&]() { + EXPECT_DEATH(registry->add([]() { return NodeData{1}; }), + "Assertion failed"); + }); } TEST_F(ThreadOwnedListTest, iterates_over_all_promises) { auto registry = MyList::make(); - auto* first_node = registry->add(NodeData{5}); - auto* second_node = registry->add(NodeData{9}); - auto* third_node = registry->add(NodeData{10}); + auto* first_node = registry->add([]() { return NodeData{5}; }); + auto* second_node = registry->add([]() { return NodeData{9}; }); + auto* third_node = registry->add([]() { return NodeData{10}; }); EXPECT_EQ(nodes_in_registry(registry), (std::vector{third_node->data.snapshot(), @@ -121,9 +123,9 @@ TEST_F(ThreadOwnedListTest, iterates_over_all_promises) { TEST_F(ThreadOwnedListTest, iterates_in_another_thread_over_all_promises) { auto registry = MyList::make(); - auto* first_node = registry->add(NodeData{19}); - auto* second_node = registry->add(NodeData{0}); - auto* third_node = registry->add(NodeData{3}); + auto* first_node = registry->add([]() { return NodeData{19}; }); + auto* second_node = registry->add([]() { return NodeData{0}; }); + auto* third_node = registry->add([]() { return NodeData{3}; }); std::thread([&]() { EXPECT_EQ(nodes_in_registry(registry), @@ -140,8 +142,8 @@ TEST_F(ThreadOwnedListTest, iterates_in_another_thread_over_all_promises) { TEST_F(ThreadOwnedListTest, marked_promises_are_deleted_in_garbage_collection) { auto registry = MyList::make(); - auto* node_to_delete = registry->add(NodeData{1}); - auto* another_node = registry->add(NodeData{77}); + auto* node_to_delete = registry->add([]() { return NodeData{1}; }); + auto* another_node = registry->add([]() { return NodeData{77}; }); registry->mark_for_deletion(node_to_delete); EXPECT_EQ(nodes_in_registry(registry), @@ -161,9 +163,9 @@ TEST_F(ThreadOwnedListTest, marked_promises_are_deleted_in_garbage_collection) { TEST_F(ThreadOwnedListTest, garbage_collection_deletes_marked_promises) { { auto registry = MyList::make(); - auto* first_node = registry->add(NodeData{21}); - auto* second_node = registry->add(NodeData{1}); - auto* third_node = registry->add(NodeData{100}); + auto* first_node = registry->add([]() { return NodeData{21}; }); + auto* second_node = registry->add([]() { return NodeData{1}; }); + auto* third_node = registry->add([]() { return NodeData{100}; }); registry->mark_for_deletion(first_node); registry->garbage_collect(); @@ -178,9 +180,9 @@ TEST_F(ThreadOwnedListTest, garbage_collection_deletes_marked_promises) { } { auto registry = MyList::make(); - auto* first_node = registry->add(NodeData{21}); - auto* second_node = registry->add(NodeData{1}); - auto* third_node = registry->add(NodeData{100}); + auto* first_node = registry->add([]() { return NodeData{21}; }); + auto* second_node = registry->add([]() { return NodeData{1}; }); + auto* third_node = registry->add([]() { return NodeData{100}; }); registry->mark_for_deletion(second_node); registry->garbage_collect(); @@ -195,9 +197,9 @@ TEST_F(ThreadOwnedListTest, garbage_collection_deletes_marked_promises) { } { auto registry = MyList::make(); - auto* first_node = registry->add(NodeData{21}); - auto* second_node = registry->add(NodeData{1}); - auto* third_node = registry->add(NodeData{100}); + auto* first_node = registry->add([]() { return NodeData{21}; }); + auto* second_node = registry->add([]() { return NodeData{1}; }); + auto* third_node = registry->add([]() { return NodeData{100}; }); registry->mark_for_deletion(third_node); registry->garbage_collect(); @@ -218,7 +220,7 @@ TEST_F(ThreadOwnedListDeathTest, auto registry = MyList::make(); auto some_other_registry = MyList::make(); - auto* promise = some_other_registry->add(NodeData{33}); + auto* promise = some_other_registry->add([]() { return NodeData{33}; }); EXPECT_DEATH(registry->mark_for_deletion(promise), "Assertion failed"); } @@ -226,8 +228,8 @@ TEST_F(ThreadOwnedListDeathTest, TEST_F(ThreadOwnedListTest, another_thread_can_mark_a_promise_for_deletion) { auto registry = MyList::make(); - auto* node_to_delete = registry->add(NodeData{7}); - auto* another_node = registry->add(NodeData{4}); + auto* node_to_delete = registry->add([]() { return NodeData{7}; }); + auto* another_node = registry->add([]() { return NodeData{4}; }); std::thread([&]() { registry->mark_for_deletion(node_to_delete); }).join(); diff --git a/tests/Futures/FutureCoroutineTest.cpp b/tests/Futures/FutureCoroutineTest.cpp index 451685a41597..43db4f01b49a 100644 --- a/tests/Futures/FutureCoroutineTest.cpp +++ b/tests/Futures/FutureCoroutineTest.cpp @@ -96,7 +96,7 @@ struct ConcurrentNoWait { auto expect_all_promises_in_state(arangodb::async_registry::State state, uint number_of_promises) { uint count = 0; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { count++; EXPECT_EQ(promise.state, state); @@ -110,7 +110,7 @@ template struct FutureCoroutineTest : ::testing::Test { void SetUp() override { arangodb::async_registry::get_thread_registry().garbage_collect(); - EXPECT_TRUE(std::holds_alternative( + EXPECT_TRUE(std::holds_alternative( *arangodb::async_registry::get_current_coroutine())); } @@ -150,7 +150,7 @@ auto find_promise_by_name(std::string_view name) -> std::optional { std::optional requested_promise = std::nullopt; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { if (promise.source_location.function_name.find(name) != std::string::npos) { @@ -198,7 +198,7 @@ TYPED_TEST( static auto waiter_fn(TestType test) -> Future { auto waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( + EXPECT_TRUE(std::holds_alternative( waiter_promise->requester)); auto fn = Functions::awaited_fn(test); @@ -218,7 +218,7 @@ TYPED_TEST( // waiter did not change waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( + EXPECT_TRUE(std::holds_alternative( waiter_promise->requester)); co_return; @@ -238,7 +238,7 @@ TYPED_TEST(FutureCoroutineTest, static auto awaited_fn(TestType test) -> Future { auto promise = find_promise_by_name("awaited_fn"); EXPECT_TRUE(promise.has_value()); - EXPECT_TRUE(std::holds_alternative( + EXPECT_TRUE(std::holds_alternative( promise->requester)); co_await test->wait; @@ -248,12 +248,12 @@ TYPED_TEST(FutureCoroutineTest, static auto waiter_fn(Future&& fn) -> Future { auto waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( + EXPECT_TRUE(std::holds_alternative( waiter_promise->requester)); auto awaited_promise = find_promise_by_name("awaited_fn"); EXPECT_TRUE(awaited_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( + EXPECT_TRUE(std::holds_alternative( waiter_promise->requester)); co_await std::move(fn); @@ -266,7 +266,7 @@ TYPED_TEST(FutureCoroutineTest, // waiter did not change waiter_promise = find_promise_by_name("waiter_fn"); EXPECT_TRUE(waiter_promise.has_value()); - EXPECT_TRUE(std::holds_alternative( + EXPECT_TRUE(std::holds_alternative( waiter_promise->requester)); co_return; diff --git a/tests/Futures/FutureTest.cpp b/tests/Futures/FutureTest.cpp index ed3ed06c285a..a81f87e16d6b 100644 --- a/tests/Futures/FutureTest.cpp +++ b/tests/Futures/FutureTest.cpp @@ -845,7 +845,7 @@ auto foo() -> Future { } auto promise_count(arangodb::async_registry::ThreadRegistry& registry) -> uint { uint promise_count = 0; - registry.for_promise([&](arangodb::async_registry::PromiseSnapshot promise) { + registry.for_node([&](arangodb::async_registry::PromiseSnapshot promise) { promise_count++; }); return promise_count; @@ -856,7 +856,7 @@ TEST(FutureTest, futures_are_registered_in_global_async_registry) { { auto x = foo(); std::vector names; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { names.push_back(promise.source_location.function_name); }); @@ -916,7 +916,7 @@ TEST(FutureTest, std::optional awaited_promise; std::optional waiter_promise; uint count = 0; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { count++; if (std::string(promise.source_location.function_name) @@ -933,7 +933,7 @@ TEST(FutureTest, EXPECT_TRUE(waiter_promise.has_value()); EXPECT_EQ(awaited_promise->requester, arangodb::async_registry::Requester{waiter_promise->id}); - EXPECT_TRUE(std::holds_alternative( + EXPECT_TRUE(std::holds_alternative( waiter_promise->requester)); } } @@ -962,7 +962,7 @@ TEST(FutureTest, collected_async_promises_in_async_registry_know_their_waiter) { std::vector awaited_promises; std::optional waiter_promise; uint count = 0; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { count++; if (std::string(promise.source_location.function_name) @@ -981,7 +981,7 @@ TEST(FutureTest, collected_async_promises_in_async_registry_know_their_waiter) { arangodb::async_registry::Requester{waiter_promise->id}); EXPECT_EQ(awaited_promises[1].requester, arangodb::async_registry::Requester{waiter_promise->id}); - EXPECT_TRUE(std::holds_alternative( + EXPECT_TRUE(std::holds_alternative( waiter_promise->requester)); } } @@ -990,7 +990,7 @@ namespace { auto expect_all_promises_in_state(arangodb::async_registry::State state, uint number_of_promises) { uint count = 0; - arangodb::async_registry::registry.for_promise( + arangodb::async_registry::registry.for_node( [&](arangodb::async_registry::PromiseSnapshot promise) { count++; EXPECT_EQ(promise.state, state); From 1b7cfbbf9408b5dc38a796438b47cf3f67ab7494 Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Wed, 2 Apr 2025 10:34:14 +0200 Subject: [PATCH 08/11] Cleanup --- arangod/AsyncRegistryServer/CMakeLists.txt | 1 + arangod/AsyncRegistryServer/Feature.cpp | 29 +--- arangod/AsyncRegistryServer/Feature.h | 36 +---- arangod/AsyncRegistryServer/Metrics.cpp | 51 ++++++ arangod/AsyncRegistryServer/Metrics.h | 62 +++++++ lib/Async/Registry/Metrics.h | 60 ------- lib/Async/Registry/promise.cpp | 11 ++ lib/Async/Registry/promise.h | 15 +- lib/Async/Registry/registry_variable.cpp | 29 +--- lib/Async/Registry/registry_variable.h | 34 +++- lib/Async/include/Async/async.h | 2 +- .../{ListOfLists.h => ListOfNonOwnedLists.h} | 40 ++--- lib/Containers/Concurrent/ThreadOwnedList.h | 153 +++++++++++------- lib/Containers/Concurrent/metrics.h | 5 - tests/Containers/Concurrent/CMakeLists.txt | 2 +- ...tsTest.cpp => ListOfNonOwnedListsTest.cpp} | 51 +----- .../Concurrent/ThreadOwnedListTest.cpp | 9 +- 17 files changed, 299 insertions(+), 291 deletions(-) create mode 100644 arangod/AsyncRegistryServer/Metrics.cpp create mode 100644 arangod/AsyncRegistryServer/Metrics.h delete mode 100644 lib/Async/Registry/Metrics.h rename lib/Containers/Concurrent/{ListOfLists.h => ListOfNonOwnedLists.h} (82%) rename tests/Containers/Concurrent/{ListOfListsTest.cpp => ListOfNonOwnedListsTest.cpp} (70%) diff --git a/arangod/AsyncRegistryServer/CMakeLists.txt b/arangod/AsyncRegistryServer/CMakeLists.txt index 3f0f39acd8d9..acccd6c5e2b1 100644 --- a/arangod/AsyncRegistryServer/CMakeLists.txt +++ b/arangod/AsyncRegistryServer/CMakeLists.txt @@ -1,5 +1,6 @@ target_sources(arangoserver PRIVATE Feature.cpp + Metrics.cpp RestHandler.cpp) target_link_libraries(arangoserver arango_async_registry_stacktrace) diff --git a/arangod/AsyncRegistryServer/Feature.cpp b/arangod/AsyncRegistryServer/Feature.cpp index a943a6a5550e..08cb4255f036 100644 --- a/arangod/AsyncRegistryServer/Feature.cpp +++ b/arangod/AsyncRegistryServer/Feature.cpp @@ -49,31 +49,6 @@ DECLARE_GAUGE( arangodb_async_existing_thread_registries, std::uint64_t, "Number of threads that started currently existing asyncronous operations"); -auto Feature::RegistryMetrics::increment_total_nodes() -> void { - promises_total->count(); -} -auto Feature::RegistryMetrics::increment_registered_nodes() -> void { - existing_promises->fetch_add(1); -} -auto Feature::RegistryMetrics::decrement_registered_nodes() -> void { - existing_promises->fetch_sub(1); -} -auto Feature::RegistryMetrics::increment_ready_for_deletion_nodes() -> void { - existing_promises->fetch_add(1); -} -auto Feature::RegistryMetrics::decrement_ready_for_deletion_nodes() -> void { - existing_promises->fetch_sub(1); -} -auto Feature::RegistryMetrics::increment_total_lists() -> void { - thread_registries_total->count(); -} -auto Feature::RegistryMetrics::increment_existing_lists() -> void { - existing_thread_registries->fetch_add(1); -} -auto Feature::RegistryMetrics::decrement_existing_lists() -> void { - existing_thread_registries->fetch_sub(1); -} - Feature::Feature(Server& server) : ArangodFeature{server, *this}, _async_mutex{_schedulerWrapper} { startsAfter(); @@ -139,6 +114,4 @@ void Feature::collectOptions(std::shared_ptr options) { R"(Each thread that is involved in the async-registry needs to garbage collect its finished async function calls regularly. This option controls how often this is done in seconds. This can possibly be performance relevant because each involved thread aquires a lock.)"); } -Feature::~Feature() { - registry.set_metrics(std::make_shared()); -} +Feature::~Feature() { registry.set_metrics(nullptr); } diff --git a/arangod/AsyncRegistryServer/Feature.h b/arangod/AsyncRegistryServer/Feature.h index 6fb37f4fa1b5..6b055729239a 100644 --- a/arangod/AsyncRegistryServer/Feature.h +++ b/arangod/AsyncRegistryServer/Feature.h @@ -23,7 +23,7 @@ #pragma once #include "Async/Registry/registry_variable.h" -#include "Metrics/Fwd.h" +#include "AsyncRegistryServer/Metrics.h" #include "Basics/FutureSharedLock.h" #include "RestServer/arangod.h" #include "Scheduler/SchedulerFeature.h" @@ -32,40 +32,6 @@ namespace arangodb::async_registry { class Feature final : public ArangodFeature { private: - struct RegistryMetrics : arangodb::containers::Metrics { - RegistryMetrics( - std::shared_ptr promises_total, - std::shared_ptr> existing_promises, - std::shared_ptr> - ready_for_deletion_promises, - std::shared_ptr thread_registries_total, - std::shared_ptr> - existing_thread_registries) - : promises_total{promises_total}, - existing_promises{existing_promises}, - ready_for_deletion_promises{ready_for_deletion_promises}, - thread_registries_total{thread_registries_total}, - existing_thread_registries{existing_thread_registries} {} - ~RegistryMetrics() = default; - auto increment_total_nodes() -> void override; - auto increment_registered_nodes() -> void override; - auto decrement_registered_nodes() -> void override; - auto increment_ready_for_deletion_nodes() -> void override; - auto decrement_ready_for_deletion_nodes() -> void override; - auto increment_total_lists() -> void override; - auto increment_existing_lists() -> void override; - auto decrement_existing_lists() -> void override; - - private: - std::shared_ptr promises_total = nullptr; - std::shared_ptr> existing_promises = nullptr; - std::shared_ptr> ready_for_deletion_promises = - nullptr; - std::shared_ptr thread_registries_total = nullptr; - std::shared_ptr> existing_thread_registries = - nullptr; - }; - static auto create_metrics(arangodb::metrics::MetricsFeature& metrics_feature) -> std::shared_ptr; struct SchedulerWrapper { diff --git a/arangod/AsyncRegistryServer/Metrics.cpp b/arangod/AsyncRegistryServer/Metrics.cpp new file mode 100644 index 000000000000..d31bc040e007 --- /dev/null +++ b/arangod/AsyncRegistryServer/Metrics.cpp @@ -0,0 +1,51 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#include "Metrics.h" + +#include "Metrics/Counter.h" +#include "Metrics/Gauge.h" + +auto RegistryMetrics::increment_total_nodes() -> void { + promises_total->count(); +} +auto RegistryMetrics::increment_registered_nodes() -> void { + existing_promises->fetch_add(1); +} +auto RegistryMetrics::decrement_registered_nodes() -> void { + existing_promises->fetch_sub(1); +} +auto RegistryMetrics::increment_ready_for_deletion_nodes() -> void { + existing_promises->fetch_add(1); +} +auto RegistryMetrics::decrement_ready_for_deletion_nodes() -> void { + existing_promises->fetch_sub(1); +} +auto RegistryMetrics::increment_total_lists() -> void { + thread_registries_total->count(); +} +auto RegistryMetrics::increment_existing_lists() -> void { + existing_thread_registries->fetch_add(1); +} +auto RegistryMetrics::decrement_existing_lists() -> void { + existing_thread_registries->fetch_sub(1); +} diff --git a/arangod/AsyncRegistryServer/Metrics.h b/arangod/AsyncRegistryServer/Metrics.h new file mode 100644 index 000000000000..c434bb1635fe --- /dev/null +++ b/arangod/AsyncRegistryServer/Metrics.h @@ -0,0 +1,62 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany +/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +/// +/// Licensed under the Business Source License 1.1 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// https://github.com/arangodb/arangodb/blob/devel/LICENSE +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Julia Volmer +//////////////////////////////////////////////////////////////////////////////// +#pragma once + +#include "Containers/Concurrent/metrics.h" +#include "Metrics/Fwd.h" + +struct RegistryMetrics : arangodb::containers::Metrics { + RegistryMetrics( + std::shared_ptr promises_total, + std::shared_ptr> + existing_promises, + std::shared_ptr> + ready_for_deletion_promises, + std::shared_ptr thread_registries_total, + std::shared_ptr> + existing_thread_registries) + : promises_total{promises_total}, + existing_promises{existing_promises}, + ready_for_deletion_promises{ready_for_deletion_promises}, + thread_registries_total{thread_registries_total}, + existing_thread_registries{existing_thread_registries} {} + ~RegistryMetrics() = default; + auto increment_total_nodes() -> void override; + auto increment_registered_nodes() -> void override; + auto decrement_registered_nodes() -> void override; + auto increment_ready_for_deletion_nodes() -> void override; + auto decrement_ready_for_deletion_nodes() -> void override; + auto increment_total_lists() -> void override; + auto increment_existing_lists() -> void override; + auto decrement_existing_lists() -> void override; + + private: + std::shared_ptr promises_total = nullptr; + std::shared_ptr> existing_promises = + nullptr; + std::shared_ptr> + ready_for_deletion_promises = nullptr; + std::shared_ptr thread_registries_total = nullptr; + std::shared_ptr> + existing_thread_registries = nullptr; +}; diff --git a/lib/Async/Registry/Metrics.h b/lib/Async/Registry/Metrics.h deleted file mode 100644 index 3133ec9d1461..000000000000 --- a/lib/Async/Registry/Metrics.h +++ /dev/null @@ -1,60 +0,0 @@ -//////////////////////////////////////////////////////////////////////////////// -/// DISCLAIMER -/// -/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany -/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany -/// -/// Licensed under the Business Source License 1.1 (the "License"); -/// you may not use this file except in compliance with the License. -/// You may obtain a copy of the License at -/// -/// https://github.com/arangodb/arangodb/blob/devel/LICENSE -/// -/// Unless required by applicable law or agreed to in writing, software -/// distributed under the License is distributed on an "AS IS" BASIS, -/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -/// See the License for the specific language governing permissions and -/// limitations under the License. -/// -/// Copyright holder is ArangoDB GmbH, Cologne, Germany -/// -/// @author Julia Volmer -//////////////////////////////////////////////////////////////////////////////// -#pragma once - -#include "Metrics/Fwd.h" - -#include -#include - -namespace arangodb::metrics { -class MetricsFeature; -} - -namespace arangodb::async_registry { - -struct Metrics { - Metrics() = default; - Metrics( - std::shared_ptr promises_total, - std::shared_ptr> existing_promises, - std::shared_ptr> - ready_for_deletion_promises, - std::shared_ptr thread_registries_total, - std::shared_ptr> existing_thread_registries) - : promises_total{promises_total}, - existing_promises{existing_promises}, - ready_for_deletion_promises{ready_for_deletion_promises}, - thread_registries_total{thread_registries_total}, - existing_thread_registries{existing_thread_registries} {} - - std::shared_ptr promises_total = nullptr; - std::shared_ptr> existing_promises = nullptr; - std::shared_ptr> ready_for_deletion_promises = - nullptr; - std::shared_ptr thread_registries_total = nullptr; - std::shared_ptr> existing_thread_registries = - nullptr; -}; - -} // namespace arangodb::async_registry diff --git a/lib/Async/Registry/promise.cpp b/lib/Async/Registry/promise.cpp index 6b30cba68174..5b2548111090 100644 --- a/lib/Async/Registry/promise.cpp +++ b/lib/Async/Registry/promise.cpp @@ -34,6 +34,17 @@ Promise::Promise(Requester requester, std::source_location entry_point) entry_point.line()}, requester{requester} {} +auto arangodb::async_registry::get_current_coroutine() noexcept -> Requester* { + struct Guard { + // initialized with current thread + Guard() : _identifier{Requester::current_thread()} {} + + Requester _identifier; + }; + static thread_local auto guard = Guard{}; + return &guard._identifier; +} + AddToAsyncRegistry::AddToAsyncRegistry(std::source_location loc) : node_in_registry{get_thread_registry().add([&]() { return Promise{*get_current_coroutine(), std::move(loc)}; diff --git a/lib/Async/Registry/promise.h b/lib/Async/Registry/promise.h index c67987cde58c..2b40c8426a5d 100644 --- a/lib/Async/Registry/promise.h +++ b/lib/Async/Registry/promise.h @@ -46,15 +46,15 @@ overloaded(Ts...) -> overloaded; namespace arangodb::async_registry { -struct ThreadRegistry; - struct SourceLocationSnapshot { std::string_view file_name; std::string_view function_name; std::uint_least32_t line; bool operator==(SourceLocationSnapshot const&) const = default; static auto from(std::source_location loc) -> SourceLocationSnapshot { - return SourceLocationSnapshot{.file_name=loc.file_name(), .function_name=loc.function_name(), .line=loc.line()}; + return SourceLocationSnapshot{.file_name = loc.file_name(), + .function_name = loc.function_name(), + .line = loc.line()}; } }; template @@ -118,7 +118,7 @@ auto inspect(Inspector& f, Requester& x) { [&](PromiseId waiter) { return RequesterWrapper{PromiseIdWrapper{waiter}}; }, - [&](basics::ThreadId waiter) { + [&](basics::ThreadId waiter) { return RequesterWrapper{ThreadIdWrapper{waiter}}; }, }, @@ -168,6 +168,12 @@ struct Promise { std::atomic state = State::Running; }; +/** + Gives a ptr to the currently running coroutine on the thread or to the + current thread if no coroutine is running at the moment. + */ +auto get_current_coroutine() noexcept -> Requester*; + struct AddToAsyncRegistry { AddToAsyncRegistry() = default; AddToAsyncRegistry(std::source_location loc); @@ -191,4 +197,3 @@ struct AddToAsyncRegistry { }; } // namespace arangodb::async_registry - diff --git a/lib/Async/Registry/registry_variable.cpp b/lib/Async/Registry/registry_variable.cpp index 01d3283dd834..58d12621fd66 100644 --- a/lib/Async/Registry/registry_variable.cpp +++ b/lib/Async/Registry/registry_variable.cpp @@ -28,39 +28,18 @@ namespace arangodb::async_registry { -containers::ListOfLists> registry; +Registry registry; -/** - Gives the thread registry of the current thread. - - Creates the thread registry when called for the first time. - */ -auto get_thread_registry() noexcept -> containers::ThreadOwnedList& { +auto get_thread_registry() noexcept -> ThreadRegistry& { struct ThreadRegistryGuard { - ThreadRegistryGuard() - : _registry{containers::ThreadOwnedList::make()} { + ThreadRegistryGuard() : _registry{ThreadRegistry::make(registry.metrics)} { registry.add(_registry); } - std::shared_ptr> _registry; + std::shared_ptr _registry; }; static thread_local auto registry_guard = ThreadRegistryGuard{}; return *registry_guard._registry; } -/** - Gives a ptr to the currently running coroutine on the thread or to the - current thread if no coroutine is running at the moment. - */ -auto get_current_coroutine() noexcept -> Requester* { - struct Guard { - // initialized with current thread - Guard() : _identifier{Requester::current_thread()} {} - - Requester _identifier; - }; - static thread_local auto guard = Guard{}; - return &guard._identifier; -} - } // namespace arangodb::async_registry diff --git a/lib/Async/Registry/registry_variable.h b/lib/Async/Registry/registry_variable.h index e34668712071..e36716ab02a8 100644 --- a/lib/Async/Registry/registry_variable.h +++ b/lib/Async/Registry/registry_variable.h @@ -23,22 +23,42 @@ #pragma once #include "Async/Registry/promise.h" -#include "Containers/Concurrent/ListOfLists.h" +#include "Containers/Concurrent/ListOfNonOwnedLists.h" #include "Containers/Concurrent/ThreadOwnedList.h" +#include "Containers/Concurrent/metrics.h" namespace arangodb::async_registry { +using ThreadRegistry = containers::ThreadOwnedList; +struct Registry : public containers::ListOfNonOwnedLists { + // all thread registries that are added to this registry will use these + // metrics + std::shared_ptr metrics; + // metrics-feature is only available after startup, therefore we need to + // update the metrics after construction + // thread registries that are added to the registry before setting the metrics + // properly are not accounted for in the metrics + auto set_metrics(std::shared_ptr new_metrics) -> void { + auto guard = std::lock_guard(_mutex); + metrics = new_metrics; + } +}; + // TODO somehow get rid of this global variable /** - Global variable that holds all coroutines. + Global variable that holds all active coroutines. + + Includes a list of coroutine thread owned lists, one for each initialized + thread. */ -extern containers::ListOfLists> registry; +extern Registry registry; /** - Get registry of all active coroutine promises on this thread. - */ -auto get_thread_registry() noexcept -> containers::ThreadOwnedList&; + Get thread registry of all active coroutine promises on current thread. -auto get_current_coroutine() noexcept -> Requester*; + Creates the thread registry when called for the first time and adds it to the + global registry. + */ +auto get_thread_registry() noexcept -> ThreadRegistry&; } // namespace arangodb::async_registry diff --git a/lib/Async/include/Async/async.h b/lib/Async/include/Async/async.h index 3b19d5880a4c..1f5c81e67d9e 100644 --- a/lib/Async/include/Async/async.h +++ b/lib/Async/include/Async/async.h @@ -33,7 +33,7 @@ struct async_promise_base : async_registry::AddToAsyncRegistry { } std::suspend_never initial_suspend() noexcept { - node_in_registry->data.state.store(async_registry::State::Running); + update_state(async_registry::State::Running); return {}; } auto final_suspend() noexcept { diff --git a/lib/Containers/Concurrent/ListOfLists.h b/lib/Containers/Concurrent/ListOfNonOwnedLists.h similarity index 82% rename from lib/Containers/Concurrent/ListOfLists.h rename to lib/Containers/Concurrent/ListOfNonOwnedLists.h index 2347875911c7..4caa3cde65f8 100644 --- a/lib/Containers/Concurrent/ListOfLists.h +++ b/lib/Containers/Concurrent/ListOfNonOwnedLists.h @@ -23,7 +23,6 @@ #pragma once #include "Containers/Concurrent/snapshot.h" -#include "Containers/Concurrent/metrics.h" #include #include @@ -46,30 +45,40 @@ concept HasExernalGarbageCollection = requires(T t) { t.garbage_collect_external(); }; -template -requires HasItemType && UpdatesMetrics && - HasExernalGarbageCollection -struct ListOfLists { - std::shared_ptr metrics; +/** + List of non-owned lists. + Does not own the inner lists, and therefore an inner list can expire at any + point. Can iterate over all elements in all lists. + */ +template +requires HasItemType && HasExernalGarbageCollection +struct ListOfNonOwnedLists { private: std::vector> _lists; + + protected: std::mutex _mutex; public: + /** + Adds a list to this list of lists. + + Removes expired inner lists. + */ auto add(std::shared_ptr list) -> void { - auto guard = std::lock_guard(_mutex); - // make sure that list uses our metrics - list->set_metrics(metrics); - if (metrics) { - metrics->increment_total_lists(); - metrics->increment_existing_lists(); + if (!list) { + return; } + auto guard = std::lock_guard(_mutex); // make sure that expired nodes are deleted std::erase_if(_lists, [&](auto const& list) { return list.expired(); }); _lists.emplace_back(list); } + /** + Executes a function on each item in each list. + */ template requires IteratorOverSnapshots auto for_node(F&& function) -> void { @@ -85,13 +94,8 @@ struct ListOfLists { } } - auto set_metrics(std::shared_ptr new_metrics) -> void { - auto guard = std::lock_guard(_mutex); - metrics = new_metrics; - } - /** - Runs an external clean up. + Executes the external garbage collection on each inner list. */ void run_external_cleanup() noexcept { auto lists = [&] { diff --git a/lib/Containers/Concurrent/ThreadOwnedList.h b/lib/Containers/Concurrent/ThreadOwnedList.h index 5af01df29339..26554f015409 100644 --- a/lib/Containers/Concurrent/ThreadOwnedList.h +++ b/lib/Containers/Concurrent/ThreadOwnedList.h @@ -29,6 +29,7 @@ #include "fmt/core.h" #include +#include #include namespace arangodb::containers { @@ -37,14 +38,26 @@ template concept CanBeSetToDeleted = requires(T t) { t.set_to_deleted(); }; + /** - This list is owned by one thread: nodes can only be added on this thread but - other threads can read the list and mark nodes for deletion. + This list is owned by one thread: nodes can only be added on its owning + thread. But other threads can read the list and mark nodes for deletion. Nodes have to manually be marked for deletion, otherwise nodes are not - deleted and therfore also not this list because each node includes a + deleted and therefore also not this list because each node includes a shared_ptr to this list. Garbage collection can run either on the owning - thread or on another thread (there are two distinct functions for gc). + thread (via garbage_collect()) or on another thread (via + garbage_collect_external()). + + A thread owned list contains an atomic list of nodes. If a nodes is marked + for deletion, it stays in this list, but is additionally added to the atomic + free list. The garbage collection goes through this free list, removes each + node in this free list from the node list (and from the free list as + well) and then destroys the node. Each node has a shared ptr to the + thread owned list, which is removed when the node is marked for deletion. + Additionally, the owning thread is supposed to have a shared_ptr to the + thread owned list. Then a thread owned list is destroyed only if both the + thread is deleted and all nodes are marked for deletion. */ template requires HasSnapshot && CanBeSetToDeleted @@ -65,7 +78,7 @@ struct ThreadOwnedList // will be deleted next round. std::atomic previous = nullptr; Node* next_to_free = nullptr; - // identifies the promise list it belongs to, to be able to mark for + // identifies the promise list it belongs to, to be able to mark itself for // deletion std::shared_ptr> list; }; @@ -73,14 +86,13 @@ struct ThreadOwnedList private: std::atomic _head = nullptr; std::atomic _free_head = nullptr; - std::mutex _mutex; // gc and reading cannot happen at same time (perhaps I - // can get rid of this with epoch based reclamation) + std::mutex _mutex; // gc and reading cannot happen at same time std::shared_ptr _metrics; public: - static auto make( - std::shared_ptr metrics = std::make_shared()) noexcept + static auto make(std::shared_ptr metrics = nullptr) noexcept -> std::shared_ptr { + // (7) - this load synchronizes with the store in (6) struct MakeShared : ThreadOwnedList { MakeShared(std::shared_ptr shared_metrics) : ThreadOwnedList(shared_metrics) {} @@ -95,24 +107,22 @@ struct ThreadOwnedList cleanup(); } - template - requires std::invocable - auto for_node(F&& function) noexcept -> void { - auto guard = std::lock_guard(_mutex); - // (2) - this load synchronizes with store in (1) and (3) - for (auto current = _head.load(std::memory_order_acquire); - current != nullptr; current = current->next) { - function(current->data.snapshot()); - } - } + /** + Adds a node to the list. + Can only be called on the owning thread, crashes + otherwise. Input data needs to be given as a callback to be able to use + data types that are non-movable and non-copyable. + */ template + requires requires(F f) { + { f() } -> std::same_as; + } auto add(F&& create_data) noexcept -> Node* { auto current_thread = basics::ThreadId::current(); ADB_PROD_ASSERT(current_thread == thread) << fmt::format( - "ThreadOwnedList::add_promise was called from thread {} but needs to " - "be " - "called from ThreadOwnedList's owning thread {}. {}", + "ThreadOwnedList::add was called from thread {} but needs to " + "be called from ThreadOwnedList's owning thread {}. {}", inspection::json(current_thread), inspection::json(thread), (void*)this); auto current_head = _head.load(std::memory_order_relaxed); @@ -120,7 +130,8 @@ struct ThreadOwnedList .next = current_head, .list = this->shared_from_this()}; if (current_head != nullptr) { - current_head->previous.store(node); // TODO memory order + // (6) - this store synchronizes with the load in (7) and (9) + current_head->previous.store(node, std::memory_order_release); } // (1) - this store synchronizes with load in (2) _head.store(node, std::memory_order_release); @@ -131,14 +142,38 @@ struct ThreadOwnedList return node; } + /** + Executes a function on each node in the list that is not yet deleted + (including nodes that are marked for deletion). + + Can be called from any thread. It makes sure that all + items stay valid during iteration (i.e. are not deleted in the meantime). + */ + template + requires std::invocable + auto for_node(F&& function) noexcept -> void { + auto guard = std::lock_guard(_mutex); + // (2) - this load synchronizes with store in (1) and (3) + for (auto current = _head.load(std::memory_order_acquire); + current != nullptr; current = current->next) { + function(current->data.snapshot()); + } + } + + /** + Marks a node in the list for deletion. + + Can be called from any thread. The node needs to be part of the list, + crashes otherwise. + */ auto mark_for_deletion(Node* node) noexcept -> void { - // makes sure that promise is really in this list + // makes sure that node is really in this list ADB_PROD_ASSERT(node->list.get() == this); node->data.set_to_deleted(); - // keep a local copy of the shared pointer. This promise might be the - // last of the registry. + // keep a local copy of the shared pointer. This node might be the + // last of the list. auto self = std::move(node->list); auto current_head = _free_head.load(std::memory_order_relaxed); @@ -148,8 +183,8 @@ struct ThreadOwnedList } while (not _free_head.compare_exchange_weak(current_head, node, std::memory_order_release, std::memory_order_acquire)); - // DO NOT access promise after this line. The owner thread might already - // be running a cleanup and promise might be deleted. + // DO NOT access node after this line. The owner thread might already + // be running a cleanup and node might be deleted. if (_metrics) { _metrics->decrement_registered_nodes(); @@ -159,45 +194,56 @@ struct ThreadOwnedList // self destroyed here. registry might be destroyed here as well. } + /** + Deletes all nodes that are marked for deletion. + + Can only be called on the owning thread, crashes otherwise. + */ auto garbage_collect() noexcept -> void { auto current_thread = basics::ThreadId::current(); ADB_PROD_ASSERT(current_thread == thread) << fmt::format( "ThreadOwnedList::garbage_collect was called from thread {} but needs " - "to " - "be called from ThreadOwnedList's owning thread {}. {}", + "to be called from ThreadOwnedList's owning thread {}. {}", inspection::json(current_thread), inspection::json(thread), (void*)this); auto guard = std::lock_guard(_mutex); cleanup(); } + /** + Runs external garbage collection. + + This can be called from any thread. Cannot delete the head of the + list, calling this will therefore result in at least one + marked-for-deletion node. + */ auto garbage_collect_external() noexcept -> void { // acquire the lock. This prevents the owning thread and the observer - // from accessing promises. Note that the owing thread only adds new - // promises to the head of the list. + // from accessing nodes. Note that the owing thread only adds new + // nodes to the head of the list. auto guard = std::lock_guard(_mutex); - // we can make the following observation. Once a promise is enqueued in the - // list, it previous and next pointer is never updated, except for the - // current head element. Also, Promises are only removed, after the mutex - // has been acquired. This implies that we can clean up all promises, that + // we can make the following observation. Once a node is enqueued in the + // list, its previous and next pointer is never updated, except for the + // current head element. Also, nodes are only removed, after the mutex + // has been acquired. This implies that we can clean up all nodes that // are not in head position right now. Node* maybe_head_ptr = nullptr; - Node *current, - *next = _free_head.exchange( - nullptr, std::memory_order_acquire); // TODO check memory order + Node* current; + Node* next = _free_head.exchange(nullptr, std::memory_order_acquire); while (next != nullptr) { current = next; next = next->next_to_free; - if (current->previous != nullptr) { // TODO memory order + // (9) - this load synchronizes with the store in (6) and (8) + if (current->previous.load(std::memory_order_release) != nullptr) { if (_metrics) { _metrics->decrement_ready_for_deletion_nodes(); } remove(current); delete current; } else { - // if this is the head of the promise list, we cannot delete it because - // additional promises could have been added in the meantime - // (if these new promises would have been marked in the meantime, they + // if this is the head of the list, we cannot delete it because + // additional nodes could have been added in the meantime + // (if these new nodes would have been marked in the meantime, they // would be in the new free list due to the exchange earlier) ADB_PROD_ASSERT(maybe_head_ptr == nullptr); maybe_head_ptr = current; @@ -210,25 +256,20 @@ struct ThreadOwnedList do { maybe_head_ptr->next_to_free = current_head; // (4) - this compare_exchange_weak synchronizes with exchange in (5) - // TODO check memory order } while (not _free_head.compare_exchange_weak( current_head, maybe_head_ptr, std::memory_order_release, std::memory_order_acquire)); } } - auto set_metrics(std::shared_ptr metrics) -> void { - _metrics = metrics; - } - private: ThreadOwnedList(std::shared_ptr metrics) noexcept : thread{basics::ThreadId::current()}, _metrics{metrics} { // is now done in ListOfLists - // if (_metrics) { - // _metrics->increment_total_lists(); - // _metrics->increment_existing_lists(); - // } + if (_metrics) { + _metrics->increment_total_lists(); + _metrics->increment_existing_lists(); + } } auto cleanup() noexcept -> void { @@ -248,15 +289,17 @@ struct ThreadOwnedList auto remove(Node* node) -> void { auto* next = node->next; - auto* previous = node->previous.load(); // TODO memory order - if (previous == nullptr) { // promise is current head + // (7) - this load synchronizes with the store in (6) and (8) + auto* previous = node->previous.load(std::memory_order_acquire); + if (previous == nullptr) { // promise is current head // (3) - this store synchronizes with the load in (2) _head.store(next, std::memory_order_release); } else { previous->next = next; } if (next != nullptr) { - next->previous = previous; // TODO memory order + // (8) - this store synchronizes with the load in (7) and (9) + next->previous.store(previous, std::memory_order_release); } } }; diff --git a/lib/Containers/Concurrent/metrics.h b/lib/Containers/Concurrent/metrics.h index ffe446974701..10c765b8d2f3 100644 --- a/lib/Containers/Concurrent/metrics.h +++ b/lib/Containers/Concurrent/metrics.h @@ -38,9 +38,4 @@ struct Metrics { virtual auto decrement_existing_lists() -> void {} }; -template -concept UpdatesMetrics = requires(T t, std::shared_ptr m) { - t.set_metrics(m); -}; - } // namespace arangodb::containers diff --git a/tests/Containers/Concurrent/CMakeLists.txt b/tests/Containers/Concurrent/CMakeLists.txt index b646bc654b59..c29bed5a64ac 100644 --- a/tests/Containers/Concurrent/CMakeLists.txt +++ b/tests/Containers/Concurrent/CMakeLists.txt @@ -1,6 +1,6 @@ target_sources(arangodbtests PRIVATE ThreadOwnedListTest.cpp - ListOfListsTest.cpp) + ListOfNonOwnedListsTest.cpp) target_link_libraries(arangodbtests arango_thread_owning_list) diff --git a/tests/Containers/Concurrent/ListOfListsTest.cpp b/tests/Containers/Concurrent/ListOfNonOwnedListsTest.cpp similarity index 70% rename from tests/Containers/Concurrent/ListOfListsTest.cpp rename to tests/Containers/Concurrent/ListOfNonOwnedListsTest.cpp index 4b26a146cac7..5c8cd01af441 100644 --- a/tests/Containers/Concurrent/ListOfListsTest.cpp +++ b/tests/Containers/Concurrent/ListOfNonOwnedListsTest.cpp @@ -20,7 +20,7 @@ /// /// @author Julia Volmer //////////////////////////////////////////////////////////////////////////////// -#include "Containers/Concurrent/ListOfLists.h" +#include "Containers/Concurrent/ListOfNonOwnedLists.h" #include #include @@ -39,28 +39,12 @@ struct MyData { auto snapshot() const -> Snapshot { return Snapshot{.number = number}; } }; -struct MyMetrics : Metrics { - size_t lists = 0; - auto increment_existing_lists() -> void override { lists++; } - auto decrement_existing_lists() -> void override { lists--; } -}; - struct MyNodeList { using Item = MyData; std::vector data; - std::shared_ptr metrics; bool isGarbageCollected = false; - MyNodeList(std::vector data) : data{std::move(data)} { - if (metrics) { - metrics->increment_existing_lists(); - } - } - ~MyNodeList() { - if (metrics) { - metrics->decrement_existing_lists(); - } - } + MyNodeList(std::vector data) : data{std::move(data)} {} template requires std::invocable auto for_node(F&& function) -> void { @@ -68,15 +52,10 @@ struct MyNodeList { function(item.snapshot()); } } - - auto set_metrics(std::shared_ptr new_metrics) -> void { - metrics = new_metrics; - } - auto garbage_collect_external() -> void { isGarbageCollected = true; } }; -using MyList = ListOfLists; +using MyList = ListOfNonOwnedLists; auto nodes_in_list(MyList& registry) -> std::vector { std::vector nodes; @@ -116,30 +95,6 @@ TEST(ListOfListsTest, iterates_over_list_items) { (std::vector{{1}, {2}, {3}, {4}, {5}, {6}})); } -TEST(ListOfListsTest, uses_list_of_lists_metrics_for_all_lists) { - MyList list; - auto inner_list = std::make_shared(std::vector{1, 3, 4}); - list.add(inner_list); - - EXPECT_EQ(dynamic_cast(list.metrics.get()), - nullptr); // uses default empty - - auto newMetrics = std::make_shared(); - list.set_metrics(newMetrics); - EXPECT_NE(dynamic_cast(list.metrics.get()), nullptr); - - auto first_inner_list = - std::make_shared(std::vector{1, 2, 3}); - auto second_inner_list = - std::make_shared(std::vector{4, 5, 6}); - list.add(first_inner_list); - list.add(second_inner_list); - - EXPECT_NE(dynamic_cast(list.metrics.get()), nullptr); - EXPECT_EQ(newMetrics->lists, 2); - EXPECT_EQ(dynamic_cast(list.metrics.get())->lists, 2); -} - TEST(ListOfListsTest, executes_garbage_collection_on_each_list) { MyList list; auto first_inner_list = diff --git a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp index 8bbff554e792..16d8e1b5c094 100644 --- a/tests/Containers/Concurrent/ThreadOwnedListTest.cpp +++ b/tests/Containers/Concurrent/ThreadOwnedListTest.cpp @@ -218,11 +218,14 @@ TEST_F(ThreadOwnedListDeathTest, unrelated_promise_cannot_be_marked_for_deletion) { GTEST_FLAG_SET(death_test_style, "threadsafe"); auto registry = MyList::make(); - auto some_other_registry = MyList::make(); + auto* promise = registry->add([]() { return NodeData{33}; }); - auto* promise = some_other_registry->add([]() { return NodeData{33}; }); + auto some_other_registry = MyList::make(); + EXPECT_DEATH(some_other_registry->mark_for_deletion(promise), + "Assertion failed"); - EXPECT_DEATH(registry->mark_for_deletion(promise), "Assertion failed"); + // correct clean up + registry->mark_for_deletion(promise); } TEST_F(ThreadOwnedListTest, another_thread_can_mark_a_promise_for_deletion) { From ca11898b3ae0aea135bcc977d9b246c651883cb6 Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Mon, 14 Apr 2025 12:55:19 +0200 Subject: [PATCH 09/11] Delete now unused files --- lib/Async/Registry/registry.cpp | 40 --- lib/Async/Registry/registry.h | 115 --------- lib/Async/Registry/thread_registry.cpp | 209 ---------------- lib/Async/Registry/thread_registry.h | 148 ------------ tests/Async/Registry/ThreadRegistryTest.cpp | 254 -------------------- 5 files changed, 766 deletions(-) delete mode 100644 lib/Async/Registry/registry.cpp delete mode 100644 lib/Async/Registry/registry.h delete mode 100644 lib/Async/Registry/thread_registry.cpp delete mode 100644 lib/Async/Registry/thread_registry.h delete mode 100644 tests/Async/Registry/ThreadRegistryTest.cpp diff --git a/lib/Async/Registry/registry.cpp b/lib/Async/Registry/registry.cpp deleted file mode 100644 index 652584c15805..000000000000 --- a/lib/Async/Registry/registry.cpp +++ /dev/null @@ -1,40 +0,0 @@ -//////////////////////////////////////////////////////////////////////////////// -/// DISCLAIMER -/// -/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany -/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany -/// -/// Licensed under the Business Source License 1.1 (the "License"); -/// you may not use this file except in compliance with the License. -/// You may obtain a copy of the License at -/// -/// https://github.com/arangodb/arangodb/blob/devel/LICENSE -/// -/// Unless required by applicable law or agreed to in writing, software -/// distributed under the License is distributed on an "AS IS" BASIS, -/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -/// See the License for the specific language governing permissions and -/// limitations under the License. -/// -/// Copyright holder is ArangoDB GmbH, Cologne, Germany -/// -/// @author Julia Volmer -//////////////////////////////////////////////////////////////////////////////// -#include "registry.h" - -#include "Async/Registry/Metrics.h" -using namespace arangodb::async_registry; - -Registry::Registry() : _metrics{std::make_shared()} {} - -auto Registry::add_thread() -> std::shared_ptr { - auto guard = std::lock_guard(mutex); - auto thread_registry = ThreadRegistry::make(_metrics, this); - registries.emplace_back(thread_registry); - return thread_registry; -} - -auto Registry::remove_thread(ThreadRegistry* registry) -> void { - auto guard = std::lock_guard(mutex); - std::erase_if(registries, [&](auto const& weak) { return weak.expired(); }); -} diff --git a/lib/Async/Registry/registry.h b/lib/Async/Registry/registry.h deleted file mode 100644 index fcaa43c57fd9..000000000000 --- a/lib/Async/Registry/registry.h +++ /dev/null @@ -1,115 +0,0 @@ -//////////////////////////////////////////////////////////////////////////////// -/// DISCLAIMER -/// -/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany -/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany -/// -/// Licensed under the Business Source License 1.1 (the "License"); -/// you may not use this file except in compliance with the License. -/// You may obtain a copy of the License at -/// -/// https://github.com/arangodb/arangodb/blob/devel/LICENSE -/// -/// Unless required by applicable law or agreed to in writing, software -/// distributed under the License is distributed on an "AS IS" BASIS, -/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -/// See the License for the specific language governing permissions and -/// limitations under the License. -/// -/// Copyright holder is ArangoDB GmbH, Cologne, Germany -/// -/// @author Julia Volmer -//////////////////////////////////////////////////////////////////////////////// -#pragma once - -#include "Async/Registry/promise.h" -#include "Async/Registry/thread_registry.h" - -#include -#include -#include - -namespace arangodb::async_registry { - -struct Metrics; - -/** - Registry of all active coroutines. - - Includes a list of coroutine thread registries, one for each initialized - thread. - */ -struct Registry { - Registry(); - - /** - Creates a new coroutine thread registry and adds it to this registry. - - Each thread needs to call this once to be able to add promises to the - coroutine registry. - */ - auto add_thread() -> std::shared_ptr; - - /** - Removes a coroutine thread registry from this registry. - - Is called when all shared pointers to the thread registry coming from the - promises are deleted as well as the thread itself (can happen in any - order). - */ - auto remove_thread(ThreadRegistry* registry) -> void; - - /** - Executes a function on each coroutine in the registry. - - Can be called from any thread. It makes sure that all - items stay valid during iteration (i.e. are not deleted in the meantime). - */ - template - requires std::invocable - auto for_promise(F&& function) -> void { - auto regs = [&] { - auto guard = std::lock_guard(mutex); - return registries; - }(); - - for (auto& registry_weak : regs) { - if (auto registry = registry_weak.lock()) { - registry->for_promise(function); - } - } - } - - /** - Exchange metrics. - - New and existing threads will use this new metrics objects. - */ - auto set_metrics(std::shared_ptr metrics) -> void { - auto guard = std::lock_guard(mutex); - _metrics = metrics; - } - - /** - Runs an external clean up. - */ - void run_external_cleanup() noexcept { - auto regs = [&] { - auto guard = std::lock_guard(mutex); - return registries; - }(); - - for (auto& registry_weak : regs) { - if (auto registry = registry_weak.lock()) { - registry->garbage_collect_external(); - } - } - } - - private: - std::vector> registries; - std::mutex mutex; - std::shared_ptr _metrics; -}; - -} // namespace arangodb::async_registry diff --git a/lib/Async/Registry/thread_registry.cpp b/lib/Async/Registry/thread_registry.cpp deleted file mode 100644 index eda43304ae2f..000000000000 --- a/lib/Async/Registry/thread_registry.cpp +++ /dev/null @@ -1,209 +0,0 @@ -//////////////////////////////////////////////////////////////////////////////// -/// DISCLAIMER -/// -/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany -/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany -/// -/// Licensed under the Business Source License 1.1 (the "License"); -/// you may not use this file except in compliance with the License. -/// You may obtain a copy of the License at -/// -/// https://github.com/arangodb/arangodb/blob/devel/LICENSE -/// -/// Unless required by applicable law or agreed to in writing, software -/// distributed under the License is distributed on an "AS IS" BASIS, -/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -/// See the License for the specific language governing permissions and -/// limitations under the License. -/// -/// Copyright holder is ArangoDB GmbH, Cologne, Germany -/// -/// @author Julia Volmer -//////////////////////////////////////////////////////////////////////////////// -#include "thread_registry.h" -#include - -#include "Assertions/ProdAssert.h" -#include "Async/Registry/Metrics.h" -#include "Async/Registry/promise.h" -#include "Async/Registry/registry.h" -#include "Basics/Thread.h" -#include "Inspection/Format.h" -#include "Logger/LogMacros.h" -#include "Metrics/Counter.h" -#include "Metrics/Gauge.h" - -using namespace arangodb::async_registry; - -auto ThreadRegistry::make(std::shared_ptr metrics, - Registry* registry) - -> std::shared_ptr { - struct MakeSharedThreadRegistry : ThreadRegistry { - MakeSharedThreadRegistry(std::shared_ptr metrics, - Registry* registry) - : ThreadRegistry{metrics, registry} {} - }; - return std::make_shared(metrics, registry); -} - -ThreadRegistry::ThreadRegistry(std::shared_ptr metrics, - Registry* registry) - : thread{ThreadId::current()}, registry{registry}, metrics{metrics} { - if (metrics->thread_registries_total != nullptr) { - metrics->thread_registries_total->count(); - } - if (metrics->existing_thread_registries != nullptr) { - metrics->existing_thread_registries->fetch_add(1); - } -} - -ThreadRegistry::~ThreadRegistry() noexcept { - if (metrics->existing_thread_registries != nullptr) { - metrics->existing_thread_registries->fetch_sub(1); - } - if (registry != nullptr) { - registry->remove_thread(this); - } - cleanup(); -} - -auto ThreadRegistry::add_promise(Requester requester, - std::source_location location) noexcept - -> Promise* { - // promise needs to live on the same thread as this registry - auto current_thread = ThreadId::current(); - ADB_PROD_ASSERT(current_thread == thread) - << "ThreadRegistry::add_promise was called from thread " - << fmt::format("{}", current_thread) - << " but needs to be called from ThreadRegistry's owning thread " - << fmt::format("{}", thread) << ". " << this; - if (metrics->promises_total != nullptr) { - metrics->promises_total->count(); - } - auto current_head = promise_head.load(std::memory_order_relaxed); - auto promise = new Promise{current_head, shared_from_this(), requester, - std::move(location)}; - if (current_head != nullptr) { - current_head->previous = promise; - } - // (1) - this store synchronizes with load in (2) - promise_head.store(promise, std::memory_order_release); - if (metrics->existing_promises != nullptr) { - metrics->existing_promises->fetch_add(1); - } - return promise; -} - -auto ThreadRegistry::mark_for_deletion(Promise* promise) noexcept -> void { - // makes sure that promise is really in this list - ADB_PROD_ASSERT(promise->registry.get() == this); - - promise->state.store(State::Deleted); - - // keep a local copy of the shared pointer. This promise might be the - // last of the registry. - auto self = std::move(promise->registry); - - auto current_head = free_head.load(std::memory_order_relaxed); - do { - promise->next_to_free = current_head; - // (4) - this compare_exchange_weak synchronizes with exchange in (5) - } while (not free_head.compare_exchange_weak(current_head, promise, - std::memory_order_release, - std::memory_order_acquire)); - // DO NOT access promise after this line. The owner thread might already - // be running a cleanup and promise might be deleted. - - if (metrics->existing_promises != nullptr) { - metrics->existing_promises->fetch_sub(1); - } - if (metrics->ready_for_deletion_promises != nullptr) { - metrics->ready_for_deletion_promises->fetch_add(1); - } - - // self destroyed here. registry might be destroyed here as well. -} - -auto ThreadRegistry::garbage_collect() noexcept -> void { - auto current_thread = ThreadId::current(); - ADB_PROD_ASSERT(current_thread == thread) - << "ThreadRegistry::garbage_collect was called from thread " - << fmt::format("{}", current_thread) - << " but needs to be called from ThreadRegistry's owning thread " - << fmt::format("{}", thread) << ". " << this; - auto guard = std::lock_guard(mutex); - cleanup(); -} - -auto ThreadRegistry::cleanup() noexcept -> void { - // (5) - this exchange synchronizes with compare_exchange_weak in (4) - Promise *current, - *next = free_head.exchange(nullptr, std::memory_order_acquire); - while (next != nullptr) { - current = next; - next = next->next_to_free; - if (metrics->ready_for_deletion_promises != nullptr) { - metrics->ready_for_deletion_promises->fetch_sub(1); - } - remove(current); - delete current; - } -} - -auto ThreadRegistry::garbage_collect_external() noexcept -> void { - // acquire the lock. This prevents the owning thread and the observer - // from accessing promises. Note that the owing thread only adds new - // promises to the head of the list. - auto guard = std::lock_guard(mutex); - // we can make the following observation. Once a promise is enqueued in the - // list, it previous and next pointer is never updated, except for the current - // head element. Also, Promises are only removed, after the mutex has been - // acquired. This implies that we can clean up all promises, that are not - // in head position right now. - Promise* maybe_head_ptr = nullptr; - Promise *current, - *next = free_head.exchange(nullptr, std::memory_order_acquire); - while (next != nullptr) { - current = next; - next = next->next_to_free; - if (current->previous != nullptr) { - if (metrics->ready_for_deletion_promises != nullptr) { - metrics->ready_for_deletion_promises->fetch_sub(1); - } - remove(current); - delete current; - } else { - // if this is the head of the promise list, we cannot delete it because - // additional promises could have been added in the meantime - // (if these new promises would have been marked in the meantime, they - // would be in the new free list due to the exchange earlier) - ADB_PROD_ASSERT(maybe_head_ptr == nullptr); - maybe_head_ptr = current; - } - } - // After the clean up we have to add the potential head back into the free - // list. - if (maybe_head_ptr) { - auto current_head = free_head.load(std::memory_order_relaxed); - do { - maybe_head_ptr->next_to_free = current_head; - // (4) - this compare_exchange_weak synchronizes with exchange in (5) - } while (not free_head.compare_exchange_weak(current_head, maybe_head_ptr, - std::memory_order_release, - std::memory_order_acquire)); - } -} - -auto ThreadRegistry::remove(Promise* promise) -> void { - auto* next = promise->next; - auto* previous = promise->previous.load(); - if (previous == nullptr) { // promise is current head - // (3) - this store synchronizes with the load in (2) - promise_head.store(next, std::memory_order_release); - } else { - previous->next = next; - } - if (next != nullptr) { - next->previous = previous; - } -} diff --git a/lib/Async/Registry/thread_registry.h b/lib/Async/Registry/thread_registry.h deleted file mode 100644 index f85a8cc44ef0..000000000000 --- a/lib/Async/Registry/thread_registry.h +++ /dev/null @@ -1,148 +0,0 @@ -//////////////////////////////////////////////////////////////////////////////// -/// DISCLAIMER -/// -/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany -/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany -/// -/// Licensed under the Business Source License 1.1 (the "License"); -/// you may not use this file except in compliance with the License. -/// You may obtain a copy of the License at -/// -/// https://github.com/arangodb/arangodb/blob/devel/LICENSE -/// -/// Unless required by applicable law or agreed to in writing, software -/// distributed under the License is distributed on an "AS IS" BASIS, -/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -/// See the License for the specific language governing permissions and -/// limitations under the License. -/// -/// Copyright holder is ArangoDB GmbH, Cologne, Germany -/// -/// @author Julia Volmer -//////////////////////////////////////////////////////////////////////////////// -#pragma once - -#include "Async/Registry/promise.h" - -#include -#include -#include -#include -#include -#include -#include "fmt/format.h" -#include "fmt/std.h" - -namespace arangodb::metrics { -template -class Gauge; -} -namespace arangodb::async_registry { - -struct Metrics; -struct Registry; - -/** - This registry belongs to a specific thread (the owning thread) and owns a - list of promises that live on this thread. - - A promise can be marked for deletion on any thread, the garbage collection - needs to be called manually and destroys all marked - promises. A promise can only be added on the owning thread, therefore adding - and garbage collection cannot happen concurrently. The garbage collection can - also not run during an iteration over all promises in the list. - - A thread registry contains an atomic list of promises. If a promise is marked - for deletion, it stays in this list, but is additionally added to the atomic - free list. The garbage collection goes through this free list, removes each - promise in this free list from the promise list (and from the free list as - well) and then destroys the promise. Each promise has a shared ptr to the - thread registry, which is removed when the promise is marked for deletion. - Additionally, the owning thread has a shared_ptr to the thread registry. - Therefore, a thread registry is destroyed only if both the thread is deleted - and all promises are marked for deletion (and when a thread registry is - destroyed, it deletes all its promises that remain in the free list). When a - thread registry is destroyed, it also removes itself from the global - registry. - */ -struct ThreadRegistry : std::enable_shared_from_this { - static auto make(std::shared_ptr metrics, - Registry* registry = nullptr) - -> std::shared_ptr; - - ~ThreadRegistry() noexcept; - - /** - Adds a promise on the registry's thread to the registry. - - Can only be called on the owning thread, crashes - otherwise. - */ - auto add_promise(Requester requester, std::source_location location) noexcept - -> Promise*; - - /** - Executes a function on each promise in the registry that is not deleted yet - (includes promises that are marked for deletion). - - Can be called from any thread. It makes sure that all - items stay valid during iteration (i.e. are not deleted in the meantime). - */ - template - requires std::invocable - auto for_promise(F&& function) noexcept -> void { - auto guard = std::lock_guard(mutex); - // (2) - this load synchronizes with store in (1) and (3) - for (auto current = promise_head.load(std::memory_order_acquire); - current != nullptr; current = current->next) { - function(current->snapshot()); - } - } - - /** - Marks a promise in the registry for deletion. - - Can be called from any thread. The promise needs to be included in - the registry list, crashes otherwise. - */ - auto mark_for_deletion(Promise* promise) noexcept -> void; - - /** - Deletes all promises that are marked for deletion. - - Can only be called on the owning thread, crashes otherwise. - */ - auto garbage_collect() noexcept -> void; - - /** - Runs external garbage collection. - - This can be called from a different thread. Cannot delete the head of the - promise list, calling this will therefore result in at least one - ready-for-deletion promise. - */ - auto garbage_collect_external() noexcept -> void; - - ThreadId const thread; - - private: - Registry* registry = nullptr; - std::atomic free_head = nullptr; - std::atomic promise_head = nullptr; - std::mutex mutex; - std::shared_ptr metrics; - - ThreadRegistry(std::shared_ptr metrics, Registry* registry); - auto cleanup() noexcept -> void; - - /** - Removes the promise from the registry. - - Caller needs to make sure that the given promise is part of this registry - (which also means that this function should only be called on the owning - thread.) - */ - auto remove(Promise* promise) -> void; -}; - -} // namespace arangodb::async_registry diff --git a/tests/Async/Registry/ThreadRegistryTest.cpp b/tests/Async/Registry/ThreadRegistryTest.cpp deleted file mode 100644 index 4396360d9b1b..000000000000 --- a/tests/Async/Registry/ThreadRegistryTest.cpp +++ /dev/null @@ -1,254 +0,0 @@ -//////////////////////////////////////////////////////////////////////////////// -/// DISCLAIMER -/// -/// Copyright 2014-2024 ArangoDB GmbH, Cologne, Germany -/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany -/// -/// Licensed under the Business Source License 1.1 (the "License"); -/// you may not use this file except in compliance with the License. -/// You may obtain a copy of the License at -/// -/// https://github.com/arangodb/arangodb/blob/devel/LICENSE -/// -/// Unless required by applicable law or agreed to in writing, software -/// distributed under the License is distributed on an "AS IS" BASIS, -/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -/// See the License for the specific language governing permissions and -/// limitations under the License. -/// -/// Copyright holder is ArangoDB GmbH, Cologne, Germany -/// -/// @author Julia Volmer -//////////////////////////////////////////////////////////////////////////////// -#include "Async/Registry/Metrics.h" -#include "Async/Registry/promise.h" -#include "Async/Registry/thread_registry.h" - -#include -#include -#include -#include - -using namespace arangodb::async_registry; - -namespace { - -auto promises_in_registry(std::shared_ptr registry) - -> std::vector { - std::vector promises; - registry->for_promise( - [&](PromiseSnapshot promise) { promises.push_back(promise); }); - return promises; -} -} // namespace - -struct AsyncThreadRegistryTest : ::testing::Test {}; -using AsyncThreadRegistryDeathTest = AsyncThreadRegistryTest; - -TEST_F(AsyncThreadRegistryTest, adds_a_promise) { - auto registry = ThreadRegistry::make(std::make_shared()); - - auto* promise_in = registry->add_promise(Requester::current_thread(), - std::source_location::current()); - - EXPECT_EQ(promises_in_registry(registry), - (std::vector{promise_in->snapshot()})); - - // make sure registry is cleaned up - promise_in->mark_for_deletion(); -} - -// TEST_F(AsyncThreadRegistryDeathTest, another_thread_cannot_add_a_promise) { -// GTEST_FLAG_SET(death_test_style, "threadsafe"); -// auto registry = ThreadRegistry::make(std::make_shared()); - -// std::jthread([&]() { -// EXPECT_DEATH(registry->add_promise(Requester::current_thread(), -// std::source_location::current()), -// "Assertion failed"); -// }); -// } - -TEST_F(AsyncThreadRegistryTest, iterates_over_all_promises) { - auto registry = ThreadRegistry::make(std::make_shared()); - - auto* first_promise = registry->add_promise(Requester::current_thread(), - std::source_location::current()); - auto* second_promise = registry->add_promise(Requester::current_thread(), - std::source_location::current()); - auto* third_promise = registry->add_promise(Requester::current_thread(), - std::source_location::current()); - - EXPECT_EQ(promises_in_registry(registry), - (std::vector{third_promise->snapshot(), - second_promise->snapshot(), - first_promise->snapshot()})); - - // make sure registry is cleaned up - first_promise->mark_for_deletion(); - second_promise->mark_for_deletion(); - third_promise->mark_for_deletion(); -} - -TEST_F(AsyncThreadRegistryTest, iterates_in_another_thread_over_all_promises) { - auto registry = ThreadRegistry::make(std::make_shared()); - - auto* first_promise = registry->add_promise(Requester::current_thread(), - std::source_location::current()); - auto* second_promise = registry->add_promise(Requester::current_thread(), - std::source_location::current()); - auto* third_promise = registry->add_promise(Requester::current_thread(), - std::source_location::current()); - - std::thread([&]() { - EXPECT_EQ(promises_in_registry(registry), - (std::vector{third_promise->snapshot(), - second_promise->snapshot(), - first_promise->snapshot()})); - }).join(); - - // make sure registry is cleaned up - first_promise->mark_for_deletion(); - second_promise->mark_for_deletion(); - third_promise->mark_for_deletion(); -} - -TEST_F(AsyncThreadRegistryTest, - marked_promises_are_deleted_in_garbage_collection) { - auto registry = ThreadRegistry::make(std::make_shared()); - auto* promise_to_delete = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - auto* another_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - - promise_to_delete->mark_for_deletion(); - EXPECT_EQ(promises_in_registry(registry), - (std::vector{another_promise->snapshot(), - promise_to_delete->snapshot()})); - - registry->garbage_collect(); - EXPECT_EQ(promises_in_registry(registry), - (std::vector{another_promise->snapshot()})); - - // make sure registry is cleaned up - another_promise->mark_for_deletion(); -} - -TEST_F(AsyncThreadRegistryTest, garbage_collection_deletes_marked_promises) { - { - auto registry = ThreadRegistry::make(std::make_shared()); - auto* first_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - auto* second_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - auto* third_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - - first_promise->mark_for_deletion(); - registry->garbage_collect(); - - EXPECT_EQ(promises_in_registry(registry), - (std::vector{third_promise->snapshot(), - second_promise->snapshot()})); - - // clean up - second_promise->mark_for_deletion(); - third_promise->mark_for_deletion(); - } - { - auto registry = ThreadRegistry::make(std::make_shared()); - auto* first_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - auto* second_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - auto* third_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - - second_promise->mark_for_deletion(); - registry->garbage_collect(); - - EXPECT_EQ(promises_in_registry(registry), - (std::vector{third_promise->snapshot(), - first_promise->snapshot()})); - - // clean up - first_promise->mark_for_deletion(); - third_promise->mark_for_deletion(); - } - { - auto registry = ThreadRegistry::make(std::make_shared()); - auto* first_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - auto* second_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - auto* third_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - - third_promise->mark_for_deletion(); - registry->garbage_collect(); - - EXPECT_EQ(promises_in_registry(registry), - (std::vector{second_promise->snapshot(), - first_promise->snapshot()})); - - // clean up - first_promise->mark_for_deletion(); - second_promise->mark_for_deletion(); - } -} - -// TEST_F(AsyncThreadRegistryDeathTest, -// unrelated_promise_cannot_be_marked_for_deletion) { -// GTEST_FLAG_SET(death_test_style, "threadsafe"); -// auto registry = ThreadRegistry::make(std::make_shared()); -// auto some_other_registry = -// ThreadRegistry::make(std::make_shared()); - -// auto* promise = -// some_other_registry->add_promise(Requester::current_thread(), -// std::source_location::current()); - -// EXPECT_DEATH(registry->mark_for_deletion(promise), "Assertion failed"); -// } - -TEST_F(AsyncThreadRegistryTest, - another_thread_can_mark_a_promise_for_deletion) { - auto registry = ThreadRegistry::make(std::make_shared()); - - auto* promise_to_delete = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - auto* another_promise = registry->add_promise( - Requester::current_thread(), std::source_location::current()); - - std::thread([&]() { promise_to_delete->mark_for_deletion(); }).join(); - - registry->garbage_collect(); - EXPECT_EQ(promises_in_registry(registry), - (std::vector{another_promise->snapshot()})); - - // clean up - another_promise->mark_for_deletion(); -} - -// TEST_F(AsyncThreadRegistryDeathTest, -// garbage_collection_for_last_promises_can_be_called_on_different_thread) -// { -// { -// auto registry = ThreadRegistry::make(std::make_shared()); - -// std::jthread( -// [&] { EXPECT_DEATH(registry->garbage_collect(), "Assertion failed"); -// }); -// } -// } - -// TEST_F(AsyncThreadRegistryDeathTest, -// garbage_collection_cannot_be_called_on_different_thread) { -// GTEST_FLAG_SET(death_test_style, "threadsafe"); - -// auto registry = ThreadRegistry::make(std::make_shared()); - -// std::jthread( -// [&] { EXPECT_DEATH(registry->garbage_collect(), "Assertion failed"); -// }); -// } From 5dbf39a5a1f0d37b6548bd319af1688ff462ee00 Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Tue, 15 Apr 2025 23:53:52 +0200 Subject: [PATCH 10/11] Fix gdb pretty printer --- .../PrettyPrinter/src/asyncregistry/gdb_data.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/arangod/AsyncRegistryServer/PrettyPrinter/src/asyncregistry/gdb_data.py b/arangod/AsyncRegistryServer/PrettyPrinter/src/asyncregistry/gdb_data.py index c7b8bd729f75..0162a3de8e69 100644 --- a/arangod/AsyncRegistryServer/PrettyPrinter/src/asyncregistry/gdb_data.py +++ b/arangod/AsyncRegistryServer/PrettyPrinter/src/asyncregistry/gdb_data.py @@ -15,7 +15,7 @@ def is_deleted(self): def __str__(self): if len(self.name) > len("arangodb::async_registry::State::"): - return self.name[len("arangodb::async_registry::State::"):] + return "\"" + self.name[len("arangodb::async_registry::State::"):] + "\"" else: return self.name @@ -85,13 +85,12 @@ class Promise: state: State @classmethod - def from_gdb(cls, value_ptr: gdb.Value): - value = value_ptr.dereference() + def from_gdb(cls, ptr: gdb.Value, value: gdb.Value): return cls( - PromiseId(value_ptr), + PromiseId(ptr), Thread.from_gdb(value["thread"]), SourceLocation.from_gdb(value["source_location"]), - Requester.from_gdb(value["requester"]['_M_i']), + Requester.from_gdb(value["requester"]["_M_i"]), State.from_gdb(value["state"]) ) @@ -119,7 +118,7 @@ class ThreadRegistry: @classmethod def from_gdb(cls, value: gdb.Value): - return cls(Promise.from_gdb(promise) for promise in GdbAtomicList(value["promise_head"]["_M_b"]["_M_p"])) + return cls(Promise.from_gdb(node_ptr, node_ptr.dereference()["data"]) for node_ptr in GdbAtomicList(value["_head"]["_M_b"]["_M_p"])) class GdbVector: def __init__(self, value: gdb.Value): @@ -129,7 +128,7 @@ def __init__(self, value: gdb.Value): def __iter__(self): current = self._begin while current != self._end: - registry = current.dereference()["_M_ptr"].dereference() + registry = current.dereference()["_M_ptr"] current += 1 yield registry @@ -139,7 +138,7 @@ class AsyncRegistry: @classmethod def from_gdb(cls, value: gdb.Value): - return cls(ThreadRegistry.from_gdb(registry) for registry in GdbVector(value['registries']['_M_impl'])) + return cls(ThreadRegistry.from_gdb(registry) for registry in GdbVector(value['_lists']['_M_impl'])) def promises(self) -> Iterable[Promise]: for registry in self.thread_registries: From 6985e030585adbe8037b3dd2854278447dbf2980 Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Tue, 22 Apr 2025 20:48:49 +0200 Subject: [PATCH 11/11] Resolve PR comments --- lib/Async/Registry/promise.cpp | 10 ++-------- lib/Containers/Concurrent/ThreadOwnedList.h | 2 +- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/Async/Registry/promise.cpp b/lib/Async/Registry/promise.cpp index 5b2548111090..f18470864b31 100644 --- a/lib/Async/Registry/promise.cpp +++ b/lib/Async/Registry/promise.cpp @@ -35,14 +35,8 @@ Promise::Promise(Requester requester, std::source_location entry_point) requester{requester} {} auto arangodb::async_registry::get_current_coroutine() noexcept -> Requester* { - struct Guard { - // initialized with current thread - Guard() : _identifier{Requester::current_thread()} {} - - Requester _identifier; - }; - static thread_local auto guard = Guard{}; - return &guard._identifier; + static thread_local auto identifier = Requester::current_thread(); + return &identifier; } AddToAsyncRegistry::AddToAsyncRegistry(std::source_location loc) diff --git a/lib/Containers/Concurrent/ThreadOwnedList.h b/lib/Containers/Concurrent/ThreadOwnedList.h index 26554f015409..7c9721e8e8af 100644 --- a/lib/Containers/Concurrent/ThreadOwnedList.h +++ b/lib/Containers/Concurrent/ThreadOwnedList.h @@ -49,7 +49,7 @@ concept CanBeSetToDeleted = requires(T t) { thread (via garbage_collect()) or on another thread (via garbage_collect_external()). - A thread owned list contains an atomic list of nodes. If a nodes is marked + A thread owned list contains an atomic list of nodes. If a node is marked for deletion, it stays in this list, but is additionally added to the atomic free list. The garbage collection goes through this free list, removes each node in this free list from the node list (and from the free list as