From 9f376228c0a9bdd01a1be071dd96a2b8147b3da3 Mon Sep 17 00:00:00 2001 From: howsohazard <143410553+howsohazard@users.noreply.github.com> Date: Fri, 2 Aug 2024 01:06:18 -0400 Subject: [PATCH] 21124: Fixes more rare memory inconsistency issues (#207) --- src/Amalgam/entity/Entity.cpp | 6 ++- .../evaluablenode/EvaluableNodeManagement.cpp | 37 ++++++++++++-- .../evaluablenode/EvaluableNodeManagement.h | 50 +++++++++++++------ .../EvaluableNodeTreeManipulation.cpp | 2 +- .../EvaluableNodeTreeManipulation.h | 2 +- .../interpreter/InterpreterOpcodesBase.cpp | 12 ++++- .../InterpreterOpcodesTransformations.cpp | 18 ++++--- 7 files changed, 94 insertions(+), 33 deletions(-) diff --git a/src/Amalgam/entity/Entity.cpp b/src/Amalgam/entity/Entity.cpp index bd18d8a8..ddd43411 100644 --- a/src/Amalgam/entity/Entity.cpp +++ b/src/Amalgam/entity/Entity.cpp @@ -270,6 +270,7 @@ bool Entity::SetValueAtLabel(StringInternPool::StringID label_sid, EvaluableNode //determine whether this label is cycle free -- if the value changes, then need to update the entity bool dest_prev_value_need_cycle_check = destination->GetNeedCycleCheck(); + bool dest_prev_value_idempotent = destination->GetIsIdempotent(); bool root_rebuilt = false; if(!direct_set) @@ -326,6 +327,7 @@ bool Entity::SetValueAtLabel(StringInternPool::StringID label_sid, EvaluableNode } bool dest_new_value_need_cycle_check = (new_value != nullptr && new_value->GetNeedCycleCheck()); + bool dest_new_value_idempotent = (new_value != nullptr && new_value->GetIsIdempotent()); if(batch_call) { @@ -336,7 +338,9 @@ bool Entity::SetValueAtLabel(StringInternPool::StringID label_sid, EvaluableNode else { //if cycle check was changed, and wasn't rebuilt, then need to do so now - if(!root_rebuilt && dest_prev_value_need_cycle_check != dest_new_value_need_cycle_check) + if(!root_rebuilt && ( + dest_prev_value_need_cycle_check != dest_new_value_need_cycle_check + || dest_prev_value_idempotent != dest_new_value_idempotent)) EvaluableNodeManager::UpdateFlagsForNodeTree(evaluableNodeManager.GetRootNode()); EntityQueryCaches *container_caches = GetContainerQueryCaches(); diff --git a/src/Amalgam/evaluablenode/EvaluableNodeManagement.cpp b/src/Amalgam/evaluablenode/EvaluableNodeManagement.cpp index 5a476e8e..a2f3fe07 100644 --- a/src/Amalgam/evaluablenode/EvaluableNodeManagement.cpp +++ b/src/Amalgam/evaluablenode/EvaluableNodeManagement.cpp @@ -12,6 +12,12 @@ Concurrency::ReadWriteMutex EvaluableNodeManager::memoryModificationMutex; #endif + +#if defined(MULTITHREAD_SUPPORT) || defined(MULTITHREAD_INTERFACE) +thread_local +#endif +EvaluableNode::ReferenceAssocType EvaluableNodeManager::nodeToParentNodeCache; + const double EvaluableNodeManager::allocExpansionFactor = 1.5; EvaluableNodeManager::~EvaluableNodeManager() @@ -895,15 +901,36 @@ void EvaluableNodeManager::MarkAllReferencedNodesInUse(size_t estimated_nodes_in } } -std::pair EvaluableNodeManager::UpdateFlagsForNodeTreeRecurse(EvaluableNode *tree, EvaluableNode::ReferenceSetType &checked) +std::pair EvaluableNodeManager::UpdateFlagsForNodeTreeRecurse(EvaluableNode *tree, + EvaluableNode *parent, EvaluableNode::ReferenceAssocType &checked_to_parent) { //attempt to insert; if new, mark as not needing a cycle check yet // though that may be changed when child nodes are evaluated below - auto [_, inserted] = checked.insert(tree); + auto [existing_record, inserted] = checked_to_parent.emplace(tree, parent); if(inserted) + { tree->SetNeedCycleCheck(false); - else //already exists, notify caller + } + else //this node has already been checked + { + //climb back up to top setting cycle checks needed + EvaluableNode *cur_node = existing_record->second; + while(cur_node != nullptr) + { + //if it's already set to cycle check, don't need to keep going + if(cur_node->GetNeedCycleCheck()) + break; + + cur_node->SetNeedCycleCheck(true); + + auto parent_record = checked_to_parent.find(cur_node); + if(parent_record != end(checked_to_parent)) + cur_node = parent_record->second; + else //shouldn't make it here + break; + } return std::make_pair(true, tree->GetIsIdempotent()); + } bool is_idempotent = (IsEvaluableNodeTypePotentiallyIdempotent(tree->GetType()) && (tree->GetNumLabels() == 0)); @@ -916,7 +943,7 @@ std::pair EvaluableNodeManager::UpdateFlagsForNodeTreeRecurse(Evalua if(cn == nullptr) continue; - auto [cn_need_cycle_check, cn_is_idempotent] = UpdateFlagsForNodeTreeRecurse(cn, checked); + auto [cn_need_cycle_check, cn_is_idempotent] = UpdateFlagsForNodeTreeRecurse(cn, tree, checked_to_parent); //update flags for tree if(cn_need_cycle_check) @@ -939,7 +966,7 @@ std::pair EvaluableNodeManager::UpdateFlagsForNodeTreeRecurse(Evalua if(cn == nullptr) continue; - auto [cn_need_cycle_check, cn_is_idempotent] = UpdateFlagsForNodeTreeRecurse(cn, checked); + auto [cn_need_cycle_check, cn_is_idempotent] = UpdateFlagsForNodeTreeRecurse(cn, tree, checked_to_parent); //update flags for tree if(cn_need_cycle_check) diff --git a/src/Amalgam/evaluablenode/EvaluableNodeManagement.h b/src/Amalgam/evaluablenode/EvaluableNodeManagement.h index 2e4d63f9..dcc33900 100644 --- a/src/Amalgam/evaluablenode/EvaluableNodeManagement.h +++ b/src/Amalgam/evaluablenode/EvaluableNodeManagement.h @@ -78,6 +78,29 @@ class EvaluableNodeReference value.nodeValue.code->SetIsIdempotent(false); } + //when attached a child node to a random location under this node, checks to see + //if all flags for this node should be rechecked + //this will update uniqueness based on the new attachment + bool NeedAllFlagsRecheckedAfterNodeAttachedAndUpdateUniqueness(EvaluableNodeReference &attached) + { + if(attached.value.nodeValue.code == nullptr) + return false; + + if(!attached.unique) + { + unique = false; + return true; + } + + if(value.nodeValue.code->GetNeedCycleCheck() != attached.value.nodeValue.code->GetNeedCycleCheck()) + return true; + + if(value.nodeValue.code->GetIsIdempotent() != attached.value.nodeValue.code->GetIsIdempotent()) + return true; + + return false; + } + //calls GetNeedCycleCheck if the reference is not nullptr, returns false if it is nullptr constexpr bool GetNeedCycleCheck() { @@ -589,19 +612,8 @@ class EvaluableNodeManager if(tree == nullptr) return; - EvaluableNode::ReferenceSetType checked; - UpdateFlagsForNodeTreeRecurse(tree, checked); - } - - //computes whether the code is cycle free and idempotent and updates all nodes appropriately - // uses checked to store nodes - static inline void UpdateFlagsForNodeTree(EvaluableNode *tree, EvaluableNode::ReferenceSetType &checked) - { - if(tree == nullptr) - return; - - checked.clear(); - UpdateFlagsForNodeTreeRecurse(tree, checked); + nodeToParentNodeCache.clear(); + UpdateFlagsForNodeTreeRecurse(tree, nullptr, nodeToParentNodeCache); } //heuristic used to determine whether unused memory should be collected (e.g., by FreeAllNodesExcept*) @@ -936,8 +948,9 @@ class EvaluableNodeManager //computes whether the code is cycle free and idempotent and updates all nodes appropriately // returns flags for whether cycle free and idempotent - // requires tree not be nullptr - static std::pair UpdateFlagsForNodeTreeRecurse(EvaluableNode *tree, EvaluableNode::ReferenceSetType &checked); + // requires tree not be nullptr; the first tree should have nullptr as parent + static std::pair UpdateFlagsForNodeTreeRecurse(EvaluableNode *tree, EvaluableNode *parent, + EvaluableNode::ReferenceAssocType &checked_to_parent); //sets or clears all referenced nodes' in use flags //if set_in_use is true, then it will set the value, if false, it will clear the value @@ -984,6 +997,13 @@ class EvaluableNodeManager //only allocated if needed std::unique_ptr nodesCurrentlyReferenced; + //buffer used for updating EvaluableNodeFlags, particularly UpdateFlagsForNodeTree +#if defined(MULTITHREAD_SUPPORT) || defined(MULTITHREAD_INTERFACE) + thread_local +#endif + static EvaluableNode::ReferenceAssocType nodeToParentNodeCache; + + //extra space to allocate when allocating static const double allocExpansionFactor; }; diff --git a/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.cpp b/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.cpp index 73883a19..372cb9b6 100644 --- a/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.cpp +++ b/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.cpp @@ -323,7 +323,7 @@ std::pair EvaluableNodeTreeManipulation::R } //things have been replaced, so anything might need to be updated - EvaluableNodeManager::UpdateFlagsForNodeTree(en, checked); + EvaluableNodeManager::UpdateFlagsForNodeTree(en); return std::make_pair(index, false); } diff --git a/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.h b/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.h index 8a86eadc..d0555e6d 100644 --- a/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.h +++ b/src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.h @@ -482,7 +482,7 @@ class EvaluableNodeTreeManipulation { EvaluableNode::ReferenceSetType checked; ReplaceLabelInTreeRecurse(tree, label_id, replacement, checked); - EvaluableNodeManager::UpdateFlagsForNodeTree(tree, checked); + EvaluableNodeManager::UpdateFlagsForNodeTree(tree); } //If the nodes, n1 and n2 can be generalized, then returns a new (allocated) node that is preferable to use (usually the more specific one) diff --git a/src/Amalgam/interpreter/InterpreterOpcodesBase.cpp b/src/Amalgam/interpreter/InterpreterOpcodesBase.cpp index fdf662c1..b50dc674 100644 --- a/src/Amalgam/interpreter/InterpreterOpcodesBase.cpp +++ b/src/Amalgam/interpreter/InterpreterOpcodesBase.cpp @@ -1185,6 +1185,8 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_SET_and_REPLACE(EvaluableN auto node_stack = CreateInterpreterNodeStackStateSaver(result); + bool result_flags_need_updates = false; + //get each address/value pair to replace in result for(size_t replace_change_index = 1; replace_change_index + 1 < ocn.size(); replace_change_index += 2) { @@ -1221,7 +1223,8 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_SET_and_REPLACE(EvaluableN node_stack.PushEvaluableNode(result); } - result.UpdatePropertiesBasedOnAttachedNode(new_value); + if(result.NeedAllFlagsRecheckedAfterNodeAttachedAndUpdateUniqueness(new_value)) + result_flags_need_updates = true; } else //en->GetType() == ENT_REPLACE { @@ -1252,10 +1255,15 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_SET_and_REPLACE(EvaluableN node_stack.PushEvaluableNode(result); } - result.UpdatePropertiesBasedOnAttachedNode(new_value); + //need to update flags because of execution happening between all + if(result.NeedAllFlagsRecheckedAfterNodeAttachedAndUpdateUniqueness(new_value)) + EvaluableNodeManager::UpdateFlagsForNodeTree(result); } } + if(result_flags_need_updates) + EvaluableNodeManager::UpdateFlagsForNodeTree(result); + return result; } diff --git a/src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp b/src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp index 97298a9e..df476fdc 100644 --- a/src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp +++ b/src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp @@ -49,7 +49,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_REWRITE(EvaluableNode *en, PopConstructionContext(); - EvaluableNodeManager::UpdateFlagsForNodeTree(result, references); + EvaluableNodeManager::UpdateFlagsForNodeTree(result); return EvaluableNodeReference(result, false); //can't make any guarantees about the new code } @@ -634,7 +634,6 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_WEAVE(EvaluableNode *en, b //find the largest of all the lists and the total number of elements size_t maximum_list_size = 0; size_t total_num_elements = 0; - bool all_lists_unique = true; for(auto &list : lists) { if(list != nullptr) @@ -642,19 +641,25 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_WEAVE(EvaluableNode *en, b size_t num_elements = list->GetOrderedChildNodes().size(); maximum_list_size = std::max(maximum_list_size, num_elements); total_num_elements += num_elements; - - all_lists_unique &= list.unique; } } //the result - EvaluableNodeReference woven_list(evaluableNodeManager->AllocNode(ENT_LIST), all_lists_unique); + EvaluableNodeReference woven_list(evaluableNodeManager->AllocNode(ENT_LIST), true); //just lists, interleave if(EvaluableNode::IsNull(function)) { woven_list->ReserveOrderedChildNodes(total_num_elements); + for(auto &list : lists) + { + if(list != nullptr && IsEvaluableNodeTypeImmediate(list->GetType())) + woven_list->SetNeedCycleCheck(true); + else + woven_list.UpdatePropertiesBasedOnAttachedNode(list); + } + //for every index, iterate over every list and if there is an element, put it in the woven list for(size_t list_index = 0; list_index < maximum_list_size; list_index++) { @@ -668,8 +673,6 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_WEAVE(EvaluableNode *en, b } } - //because each list can be unique but from the same source, still need to update all flags in case of cycle - EvaluableNodeManager::UpdateFlagsForNodeTree(woven_list); return woven_list; } @@ -712,7 +715,6 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_WEAVE(EvaluableNode *en, b evaluableNodeManager->FreeNodeIfPossible(values_to_weave); } - EvaluableNodeManager::UpdateFlagsForNodeTree(woven_list); return woven_list; }