Merge branch 'bschubert/optimize-extract-depends-node' into 'master'

rewrite _extract_depends_from_node in Cython and optimize

See merge request BuildStream/buildstream!1383

5 files changed, 59 insertions, 52 deletions

diff --git a/src/buildstream/_loader/loadelement.py b/src/buildstream/_loader/loadelement.py
index 24b4e7293..850b41a67 100644
--- a/src/buildstream/_loader/loadelement.py
+++ b/src/buildstream/_loader/loadelement.py
@@ -22,12 +22,8 @@ from itertools import count
 
 from pyroaring import BitMap, FrozenBitMap  # pylint: disable=no-name-in-module
 
-# BuildStream toplevel imports
 from .. import _yaml
 
-# Local package imports
-from .types import Symbol, Dependency
-
 
 # LoadElement():
 #
@@ -137,46 +133,3 @@ class LoadElement():
             self._dep_cache.update(elt._dep_cache)
 
         self._dep_cache = FrozenBitMap(self._dep_cache)
-
-
-# _extract_depends_from_node():
-#
-# Creates an array of Dependency objects from a given dict node 'node',
-# allows both strings and dicts for expressing the dependency and
-# throws a comprehensive LoadError in the case that the node is malformed.
-#
-# After extracting depends, the symbol is deleted from the node
-#
-# Args:
-#    node (dict): A YAML loaded dictionary
-#
-# Returns:
-#    (list): a list of Dependency objects
-#
-def _extract_depends_from_node(node, *, key=None):
-    if key is None:
-        build_depends = _extract_depends_from_node(node, key=Symbol.BUILD_DEPENDS)
-        runtime_depends = _extract_depends_from_node(node, key=Symbol.RUNTIME_DEPENDS)
-        depends = _extract_depends_from_node(node, key=Symbol.DEPENDS)
-        return build_depends + runtime_depends + depends
-    elif key == Symbol.BUILD_DEPENDS:
-        default_dep_type = Symbol.BUILD
-    elif key == Symbol.RUNTIME_DEPENDS:
-        default_dep_type = Symbol.RUNTIME
-    elif key == Symbol.DEPENDS:
-        default_dep_type = None
-    else:
-        assert False, "Unexpected value of key '{}'".format(key)
-
-    depends = _yaml.node_get(node, list, key, default_value=[])
-    output_deps = []
-
-    for index, dep in enumerate(depends):
-        dep_provenance = _yaml.node_get_provenance(node, key=key, indices=[index])
-        dependency = Dependency(dep, dep_provenance, default_dep_type=default_dep_type)
-        output_deps.append(dependency)
-
-    # Now delete the field, we dont want it anymore
-    _yaml.node_del(node, key, safe=True)
-
-    return output_deps
diff --git a/src/buildstream/_loader/loader.py b/src/buildstream/_loader/loader.py
index 5435e0d41..d52a8a72e 100644
--- a/src/buildstream/_loader/loader.py
+++ b/src/buildstream/_loader/loader.py
@@ -28,8 +28,8 @@ from .._profile import Topics, PROFILER
 from .._includes import Includes
 
 from ._loader import valid_chars_name
-from .types import Symbol
-from .loadelement import LoadElement, _extract_depends_from_node
+from .types import Symbol, extract_depends_from_node
+from .loadelement import LoadElement
 from .metaelement import MetaElement
 from .metasource import MetaSource
 from ..types import CoreWarnings
@@ -272,7 +272,7 @@ class Loader():
             ticker(filename)
 
         top_element = self._load_file_no_deps(filename, rewritable, provenance)
-        dependencies = _extract_depends_from_node(top_element.node)
+        dependencies = extract_depends_from_node(top_element.node)
         # The loader queue is a stack of tuples
         # [0] is the LoadElement instance
         # [1] is a stack of dependencies to load
@@ -306,7 +306,7 @@ class Loader():
                         # either recursively cause it to be loaded, or else we
                         # need to push this onto the loader queue in this loader
                         dep_element = self._load_file_no_deps(dep.name, rewritable, dep.provenance)
-                        dep_deps = _extract_depends_from_node(dep_element.node)
+                        dep_deps = extract_depends_from_node(dep_element.node)
                         loader_queue.append((dep_element, list(reversed(dep_deps)), []))
 
                         if _yaml.node_get(dep_element.node, str, Symbol.KIND) == 'junction':
diff --git a/src/buildstream/_loader/types.pyx b/src/buildstream/_loader/types.pyx
index d3cd06253..da33d6c54 100644
--- a/src/buildstream/_loader/types.pyx
+++ b/src/buildstream/_loader/types.pyx
@@ -120,3 +120,56 @@ cdef class Dependency:
         # Attempt to split name if no junction was specified explicitly
         if not self.junction and self.name.count(':') == 1:
             self.junction, self.name = self.name.split(':')
+
+
+# _extract_depends_from_node():
+#
+# Helper for extract_depends_from_node to get dependencies of a particular type
+#
+# Adds to an array of Dependency objects from a given dict node 'node',
+# allows both strings and dicts for expressing the dependency.
+#
+# After extracting depends, the symbol is deleted from the node
+#
+# Args:
+#    node (Node): A YAML loaded dictionary
+#    key (str): the key on the Node corresponding to the dependency type
+#    default_dep_type (str): type to give to the dependency
+#    acc (list): a list in which to add the loaded dependencies
+#
+cdef void _extract_depends_from_node(_yaml.Node node, str key, str default_dep_type, list acc) except *:
+    cdef list depends = <list> _yaml.node_get(node, list, key, None, [])
+    cdef int index
+    cdef _yaml.ProvenanceInformation dep_provenance
+
+    for index in range(len(depends)):
+        # FIXME: the provenance information would be obtainable from the Node directly if we stop
+        #        stripping provenance and have proper nodes for str elements
+        dep_provenance = <_yaml.ProvenanceInformation> _yaml.node_get_provenance(node, key=key, indices=[index])
+        dependency = Dependency(depends[index], dep_provenance, default_dep_type=default_dep_type)
+        acc.append(dependency)
+
+    # Now delete the field, we dont want it anymore
+    _yaml.node_del(node, key, safe=True)
+
+
+# extract_depends_from_node():
+#
+# Creates an array of Dependency objects from a given dict node 'node',
+# allows both strings and dicts for expressing the dependency and
+# throws a comprehensive LoadError in the case that the node is malformed.
+#
+# After extracting depends, the symbol is deleted from the node
+#
+# Args:
+#    node (Node): A YAML loaded dictionary
+#
+# Returns:
+#    (list): a list of Dependency objects
+#
+def extract_depends_from_node(_yaml.Node node):
+    cdef list acc = []
+    _extract_depends_from_node(node, <str> Symbol.BUILD_DEPENDS, <str> Symbol.BUILD, acc)
+    _extract_depends_from_node(node, <str> Symbol.RUNTIME_DEPENDS, <str> Symbol.RUNTIME, acc)
+    _extract_depends_from_node(node, <str> Symbol.DEPENDS, None, acc)
+    return acc
diff --git a/src/buildstream/_yaml.pxd b/src/buildstream/_yaml.pxd
index b6043e5a8..3cbad0d11 100644
--- a/src/buildstream/_yaml.pxd
+++ b/src/buildstream/_yaml.pxd
@@ -38,6 +38,7 @@ cdef class ProvenanceInformation:
     cdef public bint is_synthetic
 
 
+cpdef void node_del(Node node, str key, bint safe=*) except *
 cpdef object node_get(Node node, object expected_type, str key, list indices=*, object default_value=*, bint allow_none=*)
 cpdef void node_validate(Node node, list valid_keys) except *
 cpdef void node_set(Node node, object key, object value, list indices=*) except *
diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx
index edad3fbee..2122dea3a 100644
--- a/src/buildstream/_yaml.pyx
+++ b/src/buildstream/_yaml.pyx
@@ -740,7 +740,7 @@ cpdef list node_keys(Node node):
 #    key (str): The key we want to remove
 #    safe (bool): Whether to raise a KeyError if unable
 #
-def node_del(Node node, str key, bint safe=False):
+cpdef void node_del(Node node, str key, bint safe=False) except *:
     try:
         del node.value[key]
     except KeyError: