diff options
author | Jacques Lucke <jacques@blender.org> | 2021-06-17 11:43:32 +0300 |
---|---|---|
committer | Jacques Lucke <jacques@blender.org> | 2021-06-17 11:43:39 +0300 |
commit | 1388e9de8afe187f85ad857e864d7463ff8ede55 (patch) | |
tree | 78318bbe787b759898b86a2a866b4f7f097d5d10 /source | |
parent | a971409d5a85f8b5a5eb7b1b5e0a9e6b96951b88 (diff) |
Geometry Nodes: improve node locking in evaluator
This makes the parts where a node is locked more explicit. Also, now the thread
is isolated when the node is locked. This prevents some kinds of deadlocks
(which haven't happened in practice yet).
Diffstat (limited to 'source')
-rw-r--r-- | source/blender/modifiers/intern/MOD_nodes_evaluator.cc | 216 |
1 files changed, 113 insertions, 103 deletions
diff --git a/source/blender/modifiers/intern/MOD_nodes_evaluator.cc b/source/blender/modifiers/intern/MOD_nodes_evaluator.cc index 3de7497ae3f..01eebfea333 100644 --- a/source/blender/modifiers/intern/MOD_nodes_evaluator.cc +++ b/source/blender/modifiers/intern/MOD_nodes_evaluator.cc @@ -264,13 +264,10 @@ struct NodeWithState { class GeometryNodesEvaluator; /** - * Utility class that locks the state of a node. Having this is a separate class is useful because - * it allows methods to communicate that they expect the node to be locked. + * Utility class that wraps a node whose state is locked. Having this is a separate class is useful + * because it allows methods to communicate that they expect the node to be locked. */ class LockedNode : NonCopyable, NonMovable { - private: - GeometryNodesEvaluator &evaluator_; - public: /** * This is the node that is currently locked. @@ -290,13 +287,9 @@ class LockedNode : NonCopyable, NonMovable { Vector<DOutputSocket> delayed_unused_outputs; Vector<DNode> delayed_scheduled_nodes; - LockedNode(GeometryNodesEvaluator &evaluator, const DNode node, NodeState &node_state) - : evaluator_(evaluator), node(node), node_state(node_state) + LockedNode(const DNode node, NodeState &node_state) : node(node), node_state(node_state) { - node_state.mutex.lock(); } - - ~LockedNode(); }; static const CPPType *get_socket_cpp_type(const DSocket socket) @@ -570,9 +563,10 @@ class GeometryNodesEvaluator { for (const DInputSocket &socket : params_.output_sockets) { const DNode node = socket.node(); NodeState &node_state = this->get_node_state(node); - LockedNode locked_node{*this, node, node_state}; - /* Setting an input as required will schedule any linked node. */ - this->set_input_required(locked_node, socket); + this->with_locked_node(node, node_state, [&](LockedNode &locked_node) { + /* Setting an input as required will schedule any linked node. */ + this->set_input_required(locked_node, socket); + }); } } @@ -634,30 +628,33 @@ class GeometryNodesEvaluator { bool node_task_preprocessing(const DNode node, NodeState &node_state) { - LockedNode locked_node{*this, node, node_state}; - BLI_assert(node_state.schedule_state == NodeScheduleState::Scheduled); - node_state.schedule_state = NodeScheduleState::Running; + bool do_execute_node = false; + this->with_locked_node(node, node_state, [&](LockedNode &locked_node) { + BLI_assert(node_state.schedule_state == NodeScheduleState::Scheduled); + node_state.schedule_state = NodeScheduleState::Running; - /* Early return if the node has finished already. */ - if (locked_node.node_state.node_has_finished) { - return false; - } - /* Prepare outputs and check if actually any new outputs have to be computed. */ - if (!this->prepare_node_outputs_for_execution(locked_node)) { - return false; - } - /* Initialize nodes that don't support laziness. This is done after at least one output is - * required and before we check that all required inputs are provided. This reduces the - * number of "round-trips" through the task pool by one for most nodes. */ - if (!node_state.non_lazy_node_is_initialized && !node_supports_laziness(node)) { - this->initialize_non_lazy_node(locked_node); - node_state.non_lazy_node_is_initialized = true; - } - /* Prepare inputs and check if all required inputs are provided. */ - if (!this->prepare_node_inputs_for_execution(locked_node)) { - return false; - } - return true; + /* Early return if the node has finished already. */ + if (locked_node.node_state.node_has_finished) { + return; + } + /* Prepare outputs and check if actually any new outputs have to be computed. */ + if (!this->prepare_node_outputs_for_execution(locked_node)) { + return; + } + /* Initialize nodes that don't support laziness. This is done after at least one output is + * required and before we check that all required inputs are provided. This reduces the + * number of "round-trips" through the task pool by one for most nodes. */ + if (!node_state.non_lazy_node_is_initialized && !node_supports_laziness(node)) { + this->initialize_non_lazy_node(locked_node); + node_state.non_lazy_node_is_initialized = true; + } + /* Prepare inputs and check if all required inputs are provided. */ + if (!this->prepare_node_inputs_for_execution(locked_node)) { + return; + } + do_execute_node = true; + }); + return do_execute_node; } /* A node is finished when it has computed all outputs that may be used. */ @@ -898,18 +895,18 @@ class GeometryNodesEvaluator { void node_task_postprocessing(const DNode node, NodeState &node_state) { - LockedNode locked_node{*this, node, node_state}; - - const bool node_has_finished = this->finish_node_if_possible(locked_node); - const bool reschedule_requested = node_state.schedule_state == - NodeScheduleState::RunningAndRescheduled; - node_state.schedule_state = NodeScheduleState::NotScheduled; - if (reschedule_requested && !node_has_finished) { - /* Either the node rescheduled itself or another node tried to schedule it while it ran. */ - this->schedule_node(locked_node); - } + this->with_locked_node(node, node_state, [&](LockedNode &locked_node) { + const bool node_has_finished = this->finish_node_if_possible(locked_node); + const bool reschedule_requested = node_state.schedule_state == + NodeScheduleState::RunningAndRescheduled; + node_state.schedule_state = NodeScheduleState::NotScheduled; + if (reschedule_requested && !node_has_finished) { + /* Either the node rescheduled itself or another node tried to schedule it while it ran. */ + this->schedule_node(locked_node); + } - this->assert_expected_outputs_have_been_computed(locked_node); + this->assert_expected_outputs_have_been_computed(locked_node); + }); } void assert_expected_outputs_have_been_computed(LockedNode &locked_node) @@ -1097,15 +1094,16 @@ class GeometryNodesEvaluator { NodeState &node_state = this->get_node_state(node); OutputState &output_state = node_state.outputs[socket->index()]; - LockedNode locked_node{*this, node, node_state}; - if (output_state.output_usage == ValueUsage::Required) { - /* Output is marked as required already. So the node is scheduled already. */ - return; - } - /* The origin node needs to be scheduled so that it provides the requested input - * eventually. */ - output_state.output_usage = ValueUsage::Required; - this->schedule_node(locked_node); + this->with_locked_node(node, node_state, [&](LockedNode &locked_node) { + if (output_state.output_usage == ValueUsage::Required) { + /* Output is marked as required already. So the node is scheduled already. */ + return; + } + /* The origin node needs to be scheduled so that it provides the requested input + * eventually. */ + output_state.output_usage = ValueUsage::Required; + this->schedule_node(locked_node); + }); } void send_output_unused_notification(const DOutputSocket socket) @@ -1114,14 +1112,15 @@ class GeometryNodesEvaluator { NodeState &node_state = this->get_node_state(node); OutputState &output_state = node_state.outputs[socket->index()]; - LockedNode locked_node{*this, node, node_state}; - output_state.potential_users -= 1; - if (output_state.potential_users == 0) { - /* The output socket has no users anymore. */ - output_state.output_usage = ValueUsage::Unused; - /* Schedule the origin node in case it wants to set its inputs as unused as well. */ - this->schedule_node(locked_node); - } + this->with_locked_node(node, node_state, [&](LockedNode &locked_node) { + output_state.potential_users -= 1; + if (output_state.potential_users == 0) { + /* The output socket has no users anymore. */ + output_state.output_usage = ValueUsage::Unused; + /* Schedule the origin node in case it wants to set its inputs as unused as well. */ + this->schedule_node(locked_node); + } + }); } void add_node_to_task_pool(const DNode node) @@ -1249,28 +1248,27 @@ class GeometryNodesEvaluator { NodeState &node_state = this->get_node_state(node); InputState &input_state = node_state.inputs[socket->index()]; - /* Lock the node because we want to change its state. */ - LockedNode locked_node{*this, node, node_state}; - - if (socket->is_multi_input_socket()) { - /* Add a new value to the multi-input. */ - MultiInputValue &multi_value = *input_state.value.multi; - multi_value.items.append({origin, value.get()}); - } - else { - /* Assign the value to the input. */ - SingleInputValue &single_value = *input_state.value.single; - BLI_assert(single_value.value == nullptr); - single_value.value = value.get(); - } + this->with_locked_node(node, node_state, [&](LockedNode &locked_node) { + if (socket->is_multi_input_socket()) { + /* Add a new value to the multi-input. */ + MultiInputValue &multi_value = *input_state.value.multi; + multi_value.items.append({origin, value.get()}); + } + else { + /* Assign the value to the input. */ + SingleInputValue &single_value = *input_state.value.single; + BLI_assert(single_value.value == nullptr); + single_value.value = value.get(); + } - if (input_state.usage == ValueUsage::Required) { - node_state.missing_required_inputs--; - if (node_state.missing_required_inputs == 0) { - /* Schedule node if all the required inputs have been provided. */ - this->schedule_node(locked_node); + if (input_state.usage == ValueUsage::Required) { + node_state.missing_required_inputs--; + if (node_state.missing_required_inputs == 0) { + /* Schedule node if all the required inputs have been provided. */ + this->schedule_node(locked_node); + } } - } + }); } void load_unlinked_input_value(LockedNode &locked_node, @@ -1365,23 +1363,33 @@ class GeometryNodesEvaluator { { this->log_socket_value(socket, Span<GPointer>(&value, 1)); } -}; -LockedNode::~LockedNode() -{ - /* First unlock the current node. */ - node_state.mutex.unlock(); - /* Then send notifications to the other nodes. */ - for (const DOutputSocket &socket : delayed_required_outputs) { - evaluator_.send_output_required_notification(socket); - } - for (const DOutputSocket &socket : delayed_unused_outputs) { - evaluator_.send_output_unused_notification(socket); - } - for (const DNode &node : delayed_scheduled_nodes) { - evaluator_.add_node_to_task_pool(node); + /* In most cases when `NodeState` is accessed, the node has to be locked first to avoid race + * conditions. */ + template<typename Function> + void with_locked_node(const DNode node, NodeState &node_state, const Function &function) + { + LockedNode locked_node{node, node_state}; + + node_state.mutex.lock(); + /* Isolate this thread because we don't want it to start executing another node. This other + * node might want to lock the same mutex leading to a deadlock. */ + threading::isolate_task([&] { function(locked_node); }); + node_state.mutex.unlock(); + + /* Then send notifications to the other nodes after the node state is unlocked. This avoids + * locking two nodes at the same time on this thread and helps to prevent deadlocks. */ + for (const DOutputSocket &socket : locked_node.delayed_required_outputs) { + this->send_output_required_notification(socket); + } + for (const DOutputSocket &socket : locked_node.delayed_unused_outputs) { + this->send_output_unused_notification(socket); + } + for (const DNode &node : locked_node.delayed_scheduled_nodes) { + this->add_node_to_task_pool(node); + } } -} +}; NodeParamsProvider::NodeParamsProvider(GeometryNodesEvaluator &evaluator, DNode dnode, @@ -1507,8 +1515,9 @@ bool NodeParamsProvider::lazy_require_input(StringRef identifier) if (input_state.was_ready_for_execution) { return false; } - LockedNode locked_node{evaluator_, this->dnode, node_state_}; - evaluator_.set_input_required(locked_node, socket); + evaluator_.with_locked_node(this->dnode, node_state_, [&](LockedNode &locked_node) { + evaluator_.set_input_required(locked_node, socket); + }); return true; } @@ -1517,8 +1526,9 @@ void NodeParamsProvider::set_input_unused(StringRef identifier) const DInputSocket socket = this->dnode.input_by_identifier(identifier); BLI_assert(socket); - LockedNode locked_node{evaluator_, this->dnode, node_state_}; - evaluator_.set_input_unused(locked_node, socket); + evaluator_.with_locked_node(this->dnode, node_state_, [&](LockedNode &locked_node) { + evaluator_.set_input_unused(locked_node, socket); + }); } bool NodeParamsProvider::output_is_required(StringRef identifier) const |