From 6a42630a4057e704ed00fe8d478c86f774c6fd3c Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Wed, 5 Jun 2019 21:06:31 +0100 Subject: _loader/types: use type(x) is str instead of isintance We don't expected anything else than `str` or `Node`, so type() should be enough --- src/buildstream/_loader/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buildstream/_loader/types.py b/src/buildstream/_loader/types.py index f9dd38ca0..a9aa3ab32 100644 --- a/src/buildstream/_loader/types.py +++ b/src/buildstream/_loader/types.py @@ -62,7 +62,7 @@ class Dependency(): def __init__(self, dep, provenance, default_dep_type=None): self.provenance = provenance - if isinstance(dep, str): + if type(dep) is str: self.name = dep self.dep_type = default_dep_type self.junction = None -- cgit v1.2.1 From baec142e623f9e8a4efd93183ac2f7ef15d84b06 Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Wed, 5 Jun 2019 21:09:53 +0100 Subject: _loader/types: move to a cython package Types is a simple module that accounts for a few percent of a basic 'show' operation. Having it cythonized allows us to get better performance without too much wokr --- .pylintrc | 1 + setup.py | 1 + src/buildstream/_loader/types.py | 112 -------------------------------------- src/buildstream/_loader/types.pyx | 112 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 112 deletions(-) delete mode 100644 src/buildstream/_loader/types.py create mode 100644 src/buildstream/_loader/types.pyx diff --git a/.pylintrc b/.pylintrc index c33aa48b8..82afa5a1a 100644 --- a/.pylintrc +++ b/.pylintrc @@ -5,6 +5,7 @@ # run arbitrary code extension-pkg-whitelist= buildstream._loader._loader, + buildstream._loader.types, buildstream._variables, buildstream._yaml diff --git a/setup.py b/setup.py index e68dc383c..e393096b8 100755 --- a/setup.py +++ b/setup.py @@ -399,6 +399,7 @@ def register_cython_module(module_name, dependencies=None): BUILD_EXTENSIONS = [] register_cython_module("buildstream._loader._loader") +register_cython_module("buildstream._loader.types") register_cython_module("buildstream._yaml") register_cython_module("buildstream._variables", dependencies=["buildstream._yaml"]) diff --git a/src/buildstream/_loader/types.py b/src/buildstream/_loader/types.py deleted file mode 100644 index a9aa3ab32..000000000 --- a/src/buildstream/_loader/types.py +++ /dev/null @@ -1,112 +0,0 @@ -# -# Copyright (C) 2018 Codethink Limited -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU Lesser General Public -# License as published by the Free Software Foundation; either -# version 2 of the License, or (at your option) any later version. -# -# This library is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU -# Lesser General Public License for more details. -# -# You should have received a copy of the GNU Lesser General Public -# License along with this library. If not, see . -# -# Authors: -# Tristan Van Berkom - -from .._exceptions import LoadError, LoadErrorReason -from .. import _yaml - - -# Symbol(): -# -# A simple object to denote the symbols we load with from YAML -# -class Symbol(): - FILENAME = "filename" - KIND = "kind" - DEPENDS = "depends" - BUILD_DEPENDS = "build-depends" - RUNTIME_DEPENDS = "runtime-depends" - SOURCES = "sources" - CONFIG = "config" - VARIABLES = "variables" - ENVIRONMENT = "environment" - ENV_NOCACHE = "environment-nocache" - PUBLIC = "public" - TYPE = "type" - BUILD = "build" - RUNTIME = "runtime" - ALL = "all" - DIRECTORY = "directory" - JUNCTION = "junction" - SANDBOX = "sandbox" - - -# Dependency() -# -# A simple object describing a dependency -# -# Args: -# name (str): The element name -# dep_type (str): The type of dependency, can be -# Symbol.ALL, Symbol.BUILD, or Symbol.RUNTIME -# junction (str): The element name of the junction, or None -# provenance (Provenance): The YAML node provenance of where this -# dependency was declared -# -class Dependency(): - def __init__(self, dep, provenance, default_dep_type=None): - self.provenance = provenance - - if type(dep) is str: - self.name = dep - self.dep_type = default_dep_type - self.junction = None - - elif _yaml.is_node(dep): - if default_dep_type: - _yaml.node_validate(dep, ['filename', 'junction']) - dep_type = default_dep_type - else: - _yaml.node_validate(dep, ['filename', 'type', 'junction']) - - # Make type optional, for this we set it to None - dep_type = _yaml.node_get(dep, str, Symbol.TYPE, default_value=None) - if dep_type is None or dep_type == Symbol.ALL: - dep_type = None - elif dep_type not in [Symbol.BUILD, Symbol.RUNTIME]: - provenance = _yaml.node_get_provenance(dep, key=Symbol.TYPE) - raise LoadError(LoadErrorReason.INVALID_DATA, - "{}: Dependency type '{}' is not 'build', 'runtime' or 'all'" - .format(provenance, dep_type)) - - self.name = _yaml.node_get(dep, str, Symbol.FILENAME) - self.dep_type = dep_type - self.junction = _yaml.node_get(dep, str, Symbol.JUNCTION, default_value=None) - - else: - raise LoadError(LoadErrorReason.INVALID_DATA, - "{}: Dependency is not specified as a string or a dictionary".format(provenance)) - - # `:` characters are not allowed in filename if a junction was - # explicitly specified - if self.junction and ':' in self.name: - raise LoadError(LoadErrorReason.INVALID_DATA, - "{}: Dependency {} contains `:` in its name. " - "`:` characters are not allowed in filename when " - "junction attribute is specified.".format(self.provenance, self.name)) - - # Name of the element should never contain more than one `:` characters - if self.name.count(':') > 1: - raise LoadError(LoadErrorReason.INVALID_DATA, - "{}: Dependency {} contains multiple `:` in its name. " - "Recursive lookups for cross-junction elements is not " - "allowed.".format(self.provenance, self.name)) - - # 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(':') diff --git a/src/buildstream/_loader/types.pyx b/src/buildstream/_loader/types.pyx new file mode 100644 index 000000000..a9aa3ab32 --- /dev/null +++ b/src/buildstream/_loader/types.pyx @@ -0,0 +1,112 @@ +# +# Copyright (C) 2018 Codethink Limited +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library. If not, see . +# +# Authors: +# Tristan Van Berkom + +from .._exceptions import LoadError, LoadErrorReason +from .. import _yaml + + +# Symbol(): +# +# A simple object to denote the symbols we load with from YAML +# +class Symbol(): + FILENAME = "filename" + KIND = "kind" + DEPENDS = "depends" + BUILD_DEPENDS = "build-depends" + RUNTIME_DEPENDS = "runtime-depends" + SOURCES = "sources" + CONFIG = "config" + VARIABLES = "variables" + ENVIRONMENT = "environment" + ENV_NOCACHE = "environment-nocache" + PUBLIC = "public" + TYPE = "type" + BUILD = "build" + RUNTIME = "runtime" + ALL = "all" + DIRECTORY = "directory" + JUNCTION = "junction" + SANDBOX = "sandbox" + + +# Dependency() +# +# A simple object describing a dependency +# +# Args: +# name (str): The element name +# dep_type (str): The type of dependency, can be +# Symbol.ALL, Symbol.BUILD, or Symbol.RUNTIME +# junction (str): The element name of the junction, or None +# provenance (Provenance): The YAML node provenance of where this +# dependency was declared +# +class Dependency(): + def __init__(self, dep, provenance, default_dep_type=None): + self.provenance = provenance + + if type(dep) is str: + self.name = dep + self.dep_type = default_dep_type + self.junction = None + + elif _yaml.is_node(dep): + if default_dep_type: + _yaml.node_validate(dep, ['filename', 'junction']) + dep_type = default_dep_type + else: + _yaml.node_validate(dep, ['filename', 'type', 'junction']) + + # Make type optional, for this we set it to None + dep_type = _yaml.node_get(dep, str, Symbol.TYPE, default_value=None) + if dep_type is None or dep_type == Symbol.ALL: + dep_type = None + elif dep_type not in [Symbol.BUILD, Symbol.RUNTIME]: + provenance = _yaml.node_get_provenance(dep, key=Symbol.TYPE) + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Dependency type '{}' is not 'build', 'runtime' or 'all'" + .format(provenance, dep_type)) + + self.name = _yaml.node_get(dep, str, Symbol.FILENAME) + self.dep_type = dep_type + self.junction = _yaml.node_get(dep, str, Symbol.JUNCTION, default_value=None) + + else: + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Dependency is not specified as a string or a dictionary".format(provenance)) + + # `:` characters are not allowed in filename if a junction was + # explicitly specified + if self.junction and ':' in self.name: + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Dependency {} contains `:` in its name. " + "`:` characters are not allowed in filename when " + "junction attribute is specified.".format(self.provenance, self.name)) + + # Name of the element should never contain more than one `:` characters + if self.name.count(':') > 1: + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Dependency {} contains multiple `:` in its name. " + "Recursive lookups for cross-junction elements is not " + "allowed.".format(self.provenance, self.name)) + + # 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(':') -- cgit v1.2.1 From 8f6e36acea77b35f454a53383a94b8fb838957fd Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Wed, 5 Jun 2019 21:16:50 +0100 Subject: _loader/types: cimport yaml functions for better speed - _yaml: export node_validate function as Cython, as it was not done before. This requires rewriting the function to remove a closure. - Optimize node check by not calling is_node(). --- setup.py | 2 +- src/buildstream/_loader/types.pyx | 16 ++++++++-------- src/buildstream/_yaml.pxd | 1 + src/buildstream/_yaml.pyx | 13 +++++++------ 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/setup.py b/setup.py index e393096b8..ab3c6f30d 100755 --- a/setup.py +++ b/setup.py @@ -399,7 +399,7 @@ def register_cython_module(module_name, dependencies=None): BUILD_EXTENSIONS = [] register_cython_module("buildstream._loader._loader") -register_cython_module("buildstream._loader.types") +register_cython_module("buildstream._loader.types", dependencies=["buildstream._yaml"]) register_cython_module("buildstream._yaml") register_cython_module("buildstream._variables", dependencies=["buildstream._yaml"]) diff --git a/src/buildstream/_loader/types.pyx b/src/buildstream/_loader/types.pyx index a9aa3ab32..5e4e12666 100644 --- a/src/buildstream/_loader/types.pyx +++ b/src/buildstream/_loader/types.pyx @@ -18,7 +18,7 @@ # Tristan Van Berkom from .._exceptions import LoadError, LoadErrorReason -from .. import _yaml +from .. cimport _yaml # Symbol(): @@ -51,14 +51,14 @@ class Symbol(): # A simple object describing a dependency # # Args: -# name (str): The element name +# name (str or Node): The element name # dep_type (str): The type of dependency, can be # Symbol.ALL, Symbol.BUILD, or Symbol.RUNTIME # junction (str): The element name of the junction, or None -# provenance (Provenance): The YAML node provenance of where this -# dependency was declared +# provenance (ProvenanceInformation): The YAML node provenance of where this +# dependency was declared # -class Dependency(): +class Dependency: def __init__(self, dep, provenance, default_dep_type=None): self.provenance = provenance @@ -67,7 +67,7 @@ class Dependency(): self.dep_type = default_dep_type self.junction = None - elif _yaml.is_node(dep): + elif type(dep) is _yaml.Node and type(dep.value) is dict: if default_dep_type: _yaml.node_validate(dep, ['filename', 'junction']) dep_type = default_dep_type @@ -75,7 +75,7 @@ class Dependency(): _yaml.node_validate(dep, ['filename', 'type', 'junction']) # Make type optional, for this we set it to None - dep_type = _yaml.node_get(dep, str, Symbol.TYPE, default_value=None) + dep_type = _yaml.node_get(dep, str, Symbol.TYPE, None, None) if dep_type is None or dep_type == Symbol.ALL: dep_type = None elif dep_type not in [Symbol.BUILD, Symbol.RUNTIME]: @@ -86,7 +86,7 @@ class Dependency(): self.name = _yaml.node_get(dep, str, Symbol.FILENAME) self.dep_type = dep_type - self.junction = _yaml.node_get(dep, str, Symbol.JUNCTION, default_value=None) + self.junction = _yaml.node_get(dep, str, Symbol.JUNCTION, None, None) else: raise LoadError(LoadErrorReason.INVALID_DATA, diff --git a/src/buildstream/_yaml.pxd b/src/buildstream/_yaml.pxd index 27a1a888e..6a12fa7b3 100644 --- a/src/buildstream/_yaml.pxd +++ b/src/buildstream/_yaml.pxd @@ -39,6 +39,7 @@ cdef class ProvenanceInformation: 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 * cpdef list node_keys(object node) cpdef ProvenanceInformation node_get_provenance(Node node, str key=*, list indices=*) diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx index 4505e2f95..f14b55e51 100644 --- a/src/buildstream/_yaml.pyx +++ b/src/buildstream/_yaml.pyx @@ -1119,16 +1119,17 @@ cpdef object node_sanitize(object node, object dict_type=OrderedDict): # LoadError: In the case that the specified node contained # one or more invalid keys # -def node_validate(Node node, list valid_keys): +cpdef void node_validate(Node node, list valid_keys) except *: # Probably the fastest way to do this: https://stackoverflow.com/a/23062482 cdef set valid_keys_set = set(valid_keys) - invalid = next((key for key in node.value if key not in valid_keys_set), None) + cdef str key - if invalid: - provenance = node_get_provenance(node, key=invalid) - raise LoadError(LoadErrorReason.INVALID_DATA, - "{}: Unexpected key: {}".format(provenance, invalid)) + for key in node.value: + if key not in valid_keys_set: + provenance = node_get_provenance(node, key=key) + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Unexpected key: {}".format(provenance, key)) # Node copying -- cgit v1.2.1 From 8fae6d5f3182afdca1e27756d462960a1450a1d6 Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Wed, 5 Jun 2019 21:24:18 +0100 Subject: _loader/types: Make Dependency a cdef class Moving this class to Cython gives a non-negligeable speedup on 'show' operations. --- src/buildstream/_loader/types.pyx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/buildstream/_loader/types.pyx b/src/buildstream/_loader/types.pyx index 5e4e12666..de9d378e3 100644 --- a/src/buildstream/_loader/types.pyx +++ b/src/buildstream/_loader/types.pyx @@ -58,7 +58,12 @@ class Symbol(): # provenance (ProvenanceInformation): The YAML node provenance of where this # dependency was declared # -class Dependency: +cdef class Dependency: + cdef public _yaml.ProvenanceInformation provenance + cdef public str name + cdef public str dep_type + cdef public str junction + def __init__(self, dep, provenance, default_dep_type=None): self.provenance = provenance -- cgit v1.2.1 From d1748ec332a41c8d03a852f0a906bae00104001a Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Wed, 5 Jun 2019 21:32:28 +0100 Subject: _loader/types: type values wherever possible That way, cython can make better inference on the code and does not need to be too conservative. --- src/buildstream/_loader/types.pyx | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/buildstream/_loader/types.pyx b/src/buildstream/_loader/types.pyx index de9d378e3..d3cd06253 100644 --- a/src/buildstream/_loader/types.pyx +++ b/src/buildstream/_loader/types.pyx @@ -64,24 +64,29 @@ cdef class Dependency: cdef public str dep_type cdef public str junction - def __init__(self, dep, provenance, default_dep_type=None): + def __init__(self, + object dep, + _yaml.ProvenanceInformation provenance, + str default_dep_type=None): + cdef str dep_type + self.provenance = provenance if type(dep) is str: - self.name = dep + self.name = dep self.dep_type = default_dep_type self.junction = None elif type(dep) is _yaml.Node and type(dep.value) is dict: if default_dep_type: - _yaml.node_validate(dep, ['filename', 'junction']) + _yaml.node_validate(<_yaml.Node> dep, ['filename', 'junction']) dep_type = default_dep_type else: - _yaml.node_validate(dep, ['filename', 'type', 'junction']) + _yaml.node_validate(<_yaml.Node> dep, ['filename', 'type', 'junction']) # Make type optional, for this we set it to None - dep_type = _yaml.node_get(dep, str, Symbol.TYPE, None, None) - if dep_type is None or dep_type == Symbol.ALL: + dep_type = _yaml.node_get(<_yaml.Node> dep, str, Symbol.TYPE, None, None) + if dep_type is None or dep_type == Symbol.ALL: dep_type = None elif dep_type not in [Symbol.BUILD, Symbol.RUNTIME]: provenance = _yaml.node_get_provenance(dep, key=Symbol.TYPE) @@ -89,9 +94,9 @@ cdef class Dependency: "{}: Dependency type '{}' is not 'build', 'runtime' or 'all'" .format(provenance, dep_type)) - self.name = _yaml.node_get(dep, str, Symbol.FILENAME) + self.name = _yaml.node_get(<_yaml.Node> dep, str, Symbol.FILENAME) self.dep_type = dep_type - self.junction = _yaml.node_get(dep, str, Symbol.JUNCTION, None, None) + self.junction = _yaml.node_get(<_yaml.Node> dep, str, Symbol.JUNCTION, None, None) else: raise LoadError(LoadErrorReason.INVALID_DATA, -- cgit v1.2.1