From 4d93762c801229543ccc34c276710fab4586b4bc Mon Sep 17 00:00:00 2001 From: Julia Volmer Date: Mon, 19 May 2025 22:24:09 +0200 Subject: [PATCH] Get rid of parent reference when marking node for deletion --- .../include/TaskMonitoring/task.h | 1 - lib/TaskMonitoring/task.cpp | 77 ++++--------------- tests/TaskMonitoring/TaskRegistryTest.cpp | 18 ----- 3 files changed, 14 insertions(+), 82 deletions(-) diff --git a/lib/TaskMonitoring/include/TaskMonitoring/task.h b/lib/TaskMonitoring/include/TaskMonitoring/task.h index f2c07898c32a..d37c7df427ec 100644 --- a/lib/TaskMonitoring/include/TaskMonitoring/task.h +++ b/lib/TaskMonitoring/include/TaskMonitoring/task.h @@ -122,7 +122,6 @@ struct TaskInRegistry { std::string const name; std::atomic state; - std::atomic isDeleted = false; ParentTask parent; std::optional running_thread; // proably has to also be atomic because diff --git a/lib/TaskMonitoring/task.cpp b/lib/TaskMonitoring/task.cpp index 67bd84062f67..ef21b1f3b4ab 100644 --- a/lib/TaskMonitoring/task.cpp +++ b/lib/TaskMonitoring/task.cpp @@ -73,74 +73,25 @@ auto TaskInRegistry::snapshot() -> TaskSnapshot { namespace { /** - Marks itself for deletion and all finishes parent nodes that do not have any - other children. + Marks itself for deletion and deletes a parent reference. - Loops over the hierarchy of parents of the given node. This function makes - sure that these hierarchies are all marked for deletion together and can - therefore be deleted in the next garbage collection run. Otherwise, a parent - exists as long as its last child is not garbage collected because the child - owns a shared reference to the parent, requiring as many garbage collection - cycles as hierarchy levels to cleanup the given hierarchy. - - Some examples: - - If there are dependencies like (A └ B means A is parent of B) - node_A - └ node_B - └ node (input to this function) - and all of these nodes do not have any other references and are therefore - finished, then this function makes sure that all of these node are marked for - deletion. - - If we have dependencies like - node_A - ├ node_B - └ node_C - └ node (input to this function) - with node_C and node beeing finished, this function will mark node_C and node - for deletion but neither node_B nor node_A, independent of their state. + Deleting the parent reference makes sure that + a parent can directly be marked for deletion when all its children are + marked. Otherwise, we need to wait for the garbage collection to delete the + references, possibly requiring several garbage collection cycles to delete + all hierarchy levels. */ auto mark_finished_nodes_for_deletion(Node* node) { - auto current_node = node; - while (true) { - auto specific_node = - reinterpret_cast::Node*>( - current_node); - - // make sure that we don't mark a node twice for deletion - auto expected = false; - if (not specific_node->data.isDeleted.compare_exchange_strong( - expected, true, std::memory_order_acq_rel)) { - break; - } - - // do not continue if node does not have a parent - auto& parent = specific_node->data.parent; - if (not std::holds_alternative(parent)) { - specific_node->list->mark_for_deletion(specific_node); - break; - } - // do not continue if node is not the only child of parent - // if child is the only reference to parent, parent is not running any more - // (there is no Task that holds another shared reference) so no new children - // can be added to the parent - auto& parent_ref = std::get(parent); - if (parent_ref.use_count() != 1) { - // if we miss decrement updates here, it does not matter: - // miss 2 -> 1: parent mark for deletion is done by the other child - // miss 1 -> 0: loop once more but exit early because isDeleted==true - specific_node->list->mark_for_deletion(specific_node); - break; - } + auto specific_node = + reinterpret_cast::Node*>( + node); - // continue with parent node - current_node = parent_ref.get(); + // get rid of parent task and delete a shared reference + specific_node->data.parent = {RootTask{}}; - // mark node for deletion needs to be last action on specific_node, because - // then a garbage collection run can destroy the node at any time - specific_node->list->mark_for_deletion(specific_node); - } + // mark node for deletion needs to be last action on specific_node, because + // then a garbage collection run can destroy the node at any time + specific_node->list->mark_for_deletion(specific_node); } } // namespace diff --git a/tests/TaskMonitoring/TaskRegistryTest.cpp b/tests/TaskMonitoring/TaskRegistryTest.cpp index 89e192775e82..7e3df5c8e133 100644 --- a/tests/TaskMonitoring/TaskRegistryTest.cpp +++ b/tests/TaskMonitoring/TaskRegistryTest.cpp @@ -54,24 +54,6 @@ struct MyTask : public Task { struct TaskRegistryTest : ::testing::Test { void TearDown() override { - // garbage collection has to run at most twice in order to clean everything - // up on the current thread: - // - when a child task scope is deleted, the child's task-in-registry is - // marked for deletion - // - at this point its parent task scope can still exist, therefore it is - // not marked for deletion inside the child task scope destructor - // - when then the parent task scope is deleted, the parent's - // task-in-registry is still referenced by the child's task-in-registry - // (which is not yet deleted), therefore it is not yet marked for deletion - - // the first gc run destroys the child's task-in-registry - // which destroys the last reference to the parent's task-in-registry, which - // is therfore marked for deletion (together with all remaining - // task-in-registries higher up in the hierarchy that are not referenced by - // any other tasks) - get_thread_registry().garbage_collect(); - // the second gc run destroys the parent's task-in-registry (and possibly - // other marked for deletion items) get_thread_registry().garbage_collect(); EXPECT_EQ(get_all_tasks().size(), 0); }