Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

17919: Fixes rare crashes when multiple entities are being concurrently executed and garbage collection is triggered #26

Merged
merged 1 commit into from
Oct 24, 2023
Merged

17919: Fixes rare crashes when multiple entities are being concurrently executed and garbage collection is triggered #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Amalgam/entity/Entity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,8 @@ EvaluableNodeReference Entity::Execute(ExecutionCycleCount max_num_steps, Execut
Concurrency::ReadLock *locked_memory_modification_lock,
Concurrency::WriteLock *entity_write_lock,
#endif
StringInternPool::StringID label_sid, Interpreter *calling_interpreter)
StringInternPool::StringID label_sid,
Interpreter *calling_interpreter, bool copy_call_stack)
{
if(!on_self && IsLabelPrivate(label_sid))
return EvaluableNodeReference(nullptr, true);
Expand Down Expand Up @@ -468,6 +469,9 @@ EvaluableNodeReference Entity::Execute(ExecutionCycleCount max_num_steps, Execut
entity_write_lock->unlock();
#endif

if(copy_call_stack)
call_stack = evaluableNodeManager.DeepAllocCopy(call_stack);

EvaluableNodeReference retval = interpreter.ExecuteNode(node_to_execute, call_stack);
num_steps_executed = interpreter.GetNumStepsExecuted();

Expand Down
7 changes: 4 additions & 3 deletions src/Amalgam/entity/Entity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class Entity
// If locked_memory_modification_lock is specified, then it will unlock it prior to the execution, but lock it again before
// If entity_write_lock is specified, then it will unlock prior to execution after locked_memory_modification_lock is locked
// potentially writing anything out to destination_temp_enm
// If copy_call_stack is true, it will copy call_stack into the evaluableNodeManager managed by this entity while under appropriate locks
EvaluableNodeReference Execute(ExecutionCycleCount max_num_steps, ExecutionCycleCount &num_steps_executed, size_t max_num_nodes, size_t &num_nodes_allocated,
std::vector<EntityWriteListener *> *write_listeners, PrintListener *print_listener,
EvaluableNode *call_stack = nullptr, bool on_self = false, EvaluableNodeManager *destination_temp_enm = nullptr,
Expand All @@ -65,7 +66,7 @@ class Entity
Concurrency::WriteLock *entity_write_lock = nullptr,
#endif
StringInternPool::StringID label_sid = StringInternPool::NOT_A_STRING_ID,
Interpreter *calling_interpreter = nullptr);
Interpreter *calling_interpreter = nullptr, bool copy_call_stack = false);

//same as Execute but accepts a string for label name
inline EvaluableNodeReference Execute(ExecutionCycleCount max_num_steps, ExecutionCycleCount &num_steps_executed,
Expand All @@ -77,15 +78,15 @@ class Entity
Concurrency::WriteLock *entity_write_lock,
#endif
const std::string &label_name,
Interpreter *calling_interpreter = nullptr)
Interpreter *calling_interpreter = nullptr, bool copy_call_stack = false)
{
StringInternPool::StringID label_sid = string_intern_pool.GetIDFromString(label_name);
return Execute(max_num_steps, num_steps_executed, max_num_nodes, num_nodes_allocated, write_listeners, print_listener,
call_stack, on_self, destination_temp_enm,
#ifdef MULTITHREAD_SUPPORT
locked_memory_modification_lock, entity_write_lock,
#endif
label_sid, calling_interpreter);
label_sid, calling_interpreter, copy_call_stack);
}

//returns true if the entity or any of its contained entities are currently being executed, either because of multiple threads executing on it
Expand Down
4 changes: 4 additions & 0 deletions src/Amalgam/evaluablenode/EvaluableNodeManagement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
#include <vector>
#include <utility>

#ifdef MULTITHREAD_SUPPORT
Concurrency::ReadWriteMutex EvaluableNodeManager::memoryModificationMutex;
#endif

const double EvaluableNodeManager::allocExpansionFactor = 1.5;
const ExecutionCycleCountCompactDelta EvaluableNodeManager::minCycleCountBetweenGarbageCollects = 150000;

Expand Down
8 changes: 5 additions & 3 deletions src/Amalgam/evaluablenode/EvaluableNodeManagement.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,18 +620,20 @@ class EvaluableNodeManager

static void ValidateEvaluableNodeTreeMemoryIntegrityRecurse(EvaluableNode *en, EvaluableNode::ReferenceSetType &checked);

#ifdef MULTITHREAD_SUPPORT
#ifdef MULTITHREAD_SUPPORT
public:
//mutex to manage attributes of manager, including operations such as
// memory allocation, reference management, etc.
Concurrency::ReadWriteMutex managerAttributesMutex;

//mutex to manage whether memory nodes managed by this manager are being modified
//global mutex to manage whether memory nodes are being modified
//concurrent modifications can occur as long as there is only one unique thread
// that has allocated the memory
//garbage collection or destruction of the manager require a unique lock
// so that the memory can be traversed
Concurrency::ReadWriteMutex memoryModificationMutex;
//note that this is a global lock because nodes may be mixed among more than one
// EvaluableNodeManager and so garbage collection should not happening while memory is being modified
static Concurrency::ReadWriteMutex memoryModificationMutex;

protected:
#endif
Expand Down
22 changes: 6 additions & 16 deletions src/Amalgam/interpreter/InterpreterOpcodesEntityAccess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,10 +479,8 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_ENTITY_and_CALL_ENTIT
if(called_entity == nullptr)
return EvaluableNodeReference::Null();

EvaluableNodeManager *called_entity_enm = &called_entity->evaluableNodeManager;

//if have arguments, use them
EvaluableNodeReference call_stack = ConvertArgsToCallStack(args, called_entity_enm);
EvaluableNodeReference call_stack = ConvertArgsToCallStack(args, evaluableNodeManager);
node_stack.PushEvaluableNode(call_stack);

//current pointer to write listeners
Expand Down Expand Up @@ -532,8 +530,8 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_ENTITY_and_CALL_ENTIT
retval.SetNeedCycleCheck(true); //can't count on that due to things written in the write listener
}

//ConvertArgsToCallStack always adds an outer list that is safe to free using called_entity_enm
called_entity_enm->FreeNode(call_stack);
//ConvertArgsToCallStack always adds an outer list that is safe to free
evaluableNodeManager->FreeNode(call_stack);

if(_label_profiling_enabled)
PerformanceProfiler::EndOperation(evaluableNodeManager->GetNumberOfUsedNodes());
Expand Down Expand Up @@ -593,26 +591,18 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_CONTAINER(EvaluableNo
num_nodes_allowed_specified = true;
}

//use the container's EvaluableNodeManager to make sure that an outer entity
// does not free a node that an inner entity is using, which can occur when the inner
// entity is calling its container and the container frees the node
EvaluableNodeManager *container_enm = &container->evaluableNodeManager;

//attempt to get arguments
EvaluableNodeReference args = EvaluableNodeReference::Null();
if(ocn.size() > 1)
{
args = InterpretNodeForImmediateUse(ocn[1]);
args = container_enm->DeepAllocCopy(args);
}

//need to create arguments regardless
EvaluableNodeReference call_stack = ConvertArgsToCallStack(args, container_enm);
EvaluableNodeReference call_stack = ConvertArgsToCallStack(args, evaluableNodeManager);

auto node_stack = CreateInterpreterNodeStackStateSaver(call_stack);

//add accessing_entity to arguments. If accessing_entity already specified (it shouldn't be), let garbage collection clean it up
args->SetMappedChildNode(ENBISI_accessing_entity, container_enm->AllocNode(ENT_STRING, cur_entity_sid));
args->SetMappedChildNode(ENBISI_accessing_entity, evaluableNodeManager->AllocNode(ENT_STRING, cur_entity_sid));

//compute execution limits
if(AllowUnlimitedExecutionSteps() && (!num_steps_allowed_specified || num_steps_allowed == 0))
Expand Down Expand Up @@ -640,7 +630,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_CONTAINER(EvaluableNo
#ifdef MULTITHREAD_SUPPORT
&memoryModificationLock, &container.lock,
#endif
container_label_name, this);
container_label_name, this, true);

//accumulate costs of execution
curExecutionStep += num_steps_executed;
Expand Down