Small optimizationsbschubert/optimize-mapping-node

3 files changed, 37 insertions, 35 deletions

diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx
index 26c728681..c94b78ed6 100644
--- a/src/buildstream/_yaml.pyx
+++ b/src/buildstream/_yaml.pyx
@@ -32,7 +32,7 @@ from ruamel import yaml
 from ._exceptions import LoadError
 from .exceptions import LoadErrorReason
 from . cimport node
-from .node cimport MappingNode, ScalarNode, SequenceNode
+from .node cimport MappingNode, Node, ScalarNode, SequenceNode
 
 
 # These exceptions are intended to be caught entirely within
@@ -188,7 +188,7 @@ cdef class Representer:
     cdef RepresenterState _handle_wait_value_MappingStartEvent(self, object ev):
         cdef RepresenterState new_state = self._handle_doc_MappingStartEvent(ev)
         key = self.keys.pop()
-        (<MappingNode> self.output[-2])._set(key, self.output[-1])
+        (<MappingNode> self.output[-2])._set(key, <Node> self.output[-1])
         return new_state
 
     cdef RepresenterState _handle_wait_key_MappingEndEvent(self, object ev):
@@ -206,7 +206,7 @@ cdef class Representer:
     cdef RepresenterState _handle_wait_value_SequenceStartEvent(self, object ev):
         self.output.append(SequenceNode.__new__(
             SequenceNode, self._file_index, ev.start_mark.line, ev.start_mark.column, []))
-        (<MappingNode> self.output[-2])._set(self.keys[-1], self.output[-1])
+        (<MappingNode> self.output[-2])._set(self.keys[-1], <Node> self.output[-1])
         return RepresenterState.wait_list_item
 
     cdef RepresenterState _handle_wait_list_item_SequenceStartEvent(self, object ev):
diff --git a/src/buildstream/node.pxd b/src/buildstream/node.pxd
index 769de9adf..ca773b422 100644
--- a/src/buildstream/node.pxd
+++ b/src/buildstream/node.pxd
@@ -71,7 +71,7 @@ cdef class MappingNode(Node):
 
     # Private Methods
     cdef void __composite(self, MappingNode target, list path) except *
-    cdef void _set(self, str key, object value)
+    cdef void _set(self, str key, Node value)
     cdef Node _get(self, str key, default, default_constructor)
 
 
diff --git a/src/buildstream/node.pyx b/src/buildstream/node.pyx
index 42a0794c8..406bbce1b 100644
--- a/src/buildstream/node.pyx
+++ b/src/buildstream/node.pyx
@@ -504,7 +504,37 @@ cdef class MappingNode(Node):
         del self.value[key]
 
     def __setitem__(self, str key, object value):
-        self._set(key, value)
+        cdef Node old_value
+        cdef Node node
+
+        if type(value) not in [MappingNode, ScalarNode, SequenceNode]:
+            node = __create_node_recursive(value, self)
+
+            # FIXME: Do we really want to override provenance?
+            #
+            # Related to https://gitlab.com/BuildStream/buildstream/issues/1058
+            #
+            # There are only two cases were nodes are set in the code (hence without provenance):
+            #   - When automatic variables are set by the core (e-g: max-jobs)
+            #   - when plugins call Element.set_public_data
+            #
+            # The first case should never throw errors, so it is of limited interests.
+            #
+            # The second is more important. What should probably be done here is to have 'set_public_data'
+            # able of creating a fake provenance with the name of the plugin, the project and probably the
+            # element name.
+            #
+            # We would therefore have much better error messages, and would be able to get rid of most synthetic
+            # nodes.
+            old_value = <Node> self.value.get(key)
+            if old_value:
+                node.file_index = old_value.file_index
+                node.line = old_value.line
+                node.column = old_value.column
+        else:
+            node = value
+
+        self._set(key, node)
 
     #############################################################
     #                       Public Methods                      #
@@ -1108,38 +1138,10 @@ cdef class MappingNode(Node):
     #   key (str): the key for which to set the value
     #   value (object): the value to set
     #
-    cdef void _set(self, str key, object value):
+    cdef void _set(self, str key, Node value):
         cdef Node old_value
         cdef intern_key = sys.intern(key)
-
-        if type(value) in [MappingNode, ScalarNode, SequenceNode]:
-            self.value[intern_key] = value
-        else:
-            node = __create_node_recursive(value, self)
-
-            # FIXME: Do we really want to override provenance?
-            #
-            # Related to https://gitlab.com/BuildStream/buildstream/issues/1058
-            #
-            # There are only two cases were nodes are set in the code (hence without provenance):
-            #   - When automatic variables are set by the core (e-g: max-jobs)
-            #   - when plugins call Element.set_public_data
-            #
-            # The first case should never throw errors, so it is of limited interests.
-            #
-            # The second is more important. What should probably be done here is to have 'set_public_data'
-            # able of creating a fake provenance with the name of the plugin, the project and probably the
-            # element name.
-            #
-            # We would therefore have much better error messages, and would be able to get rid of most synthetic
-            # nodes.
-            old_value = self.value.get(intern_key)
-            if old_value:
-                node.file_index = old_value.file_index
-                node.line = old_value.line
-                node.column = old_value.column
-
-            self.value[intern_key] = node
+        self.value[intern_key] = value
 
     # _get(key, default, default_constructor)
     #