21124: Fixes more rare memory inconsistency issues (#207)

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();

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<bool, bool> EvaluableNodeManager::UpdateFlagsForNodeTreeRecurse(EvaluableNode *tree, EvaluableNode::ReferenceSetType &checked)
+std::pair<bool, bool> 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<bool, bool> 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<bool, bool> 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)

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<bool, bool> UpdateFlagsForNodeTreeRecurse(EvaluableNode *tree, EvaluableNode::ReferenceSetType &checked);
+	// requires tree not be nullptr; the first tree should have nullptr as parent
+	static std::pair<bool, bool> 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<NodesReferenced> 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;
 };

src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.cpp

@@ -323,7 +323,7 @@ std::pair<EvaluableNode::LabelsAssocType, bool> 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);
 }

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)

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;
 }
 

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,27 +634,32 @@ 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)
 		{
 			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;
 }