You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by no...@apache.org on 2020/12/29 12:35:13 UTC
[buildstream] branch bschubert/optimize-mapping-node created (now
cb34c95)
This is an automated email from the ASF dual-hosted git repository.
not-in-ldap pushed a change to branch bschubert/optimize-mapping-node
in repository https://gitbox.apache.org/repos/asf/buildstream.git.
at cb34c95 Small optimizations
This branch includes the following new commits:
new 712e6b6 Node code: Node now has internal _set() method to set values
new cb34c95 Small optimizations
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[buildstream] 02/02: Small optimizations
Posted by no...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
not-in-ldap pushed a commit to branch bschubert/optimize-mapping-node
in repository https://gitbox.apache.org/repos/asf/buildstream.git
commit cb34c95009f8a77ea28b7390a5abbbcc428aa171
Author: Benjamin Schubert <co...@benschubert.me>
AuthorDate: Mon Jun 29 13:55:17 2020 +0000
Small optimizations
---
src/buildstream/_yaml.pyx | 6 ++---
src/buildstream/node.pxd | 2 +-
src/buildstream/node.pyx | 64 ++++++++++++++++++++++++-----------------------
3 files changed, 37 insertions(+), 35 deletions(-)
diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx
index 26c7286..c94b78e 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 769de9a..ca773b4 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 42a0794..406bbce 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)
#
[buildstream] 01/02: Node code: Node now has internal _set() method
to set values
Posted by no...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
not-in-ldap pushed a commit to branch bschubert/optimize-mapping-node
in repository https://gitbox.apache.org/repos/asf/buildstream.git
commit 712e6b642e5263a24069f8b2300bd0cd76da1a6f
Author: Tristan van Berkom <tr...@codethink.co.uk>
AuthorDate: Mon Jun 29 22:31:11 2020 +0900
Node code: Node now has internal _set() method to set values
Summary of changes:
* node.[pyx,pxd]: Added _set() interface
Now __setitem__ uses _set() and the content is moved to the
internal cdef _set() interface.
This is a single point for setting values on dictionaries, and
now ensures that all MappingNode keys use interned strings.
* _yaml.pyx: Use _set() instead of accessing the MappingNode.value
directly.
---
src/buildstream/_yaml.pyx | 12 +++++---
src/buildstream/node.pxd | 1 +
src/buildstream/node.pyx | 75 ++++++++++++++++++++++++++++-------------------
3 files changed, 54 insertions(+), 34 deletions(-)
diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx
index 1e59b2a..26c7286 100644
--- a/src/buildstream/_yaml.pyx
+++ b/src/buildstream/_yaml.pyx
@@ -177,14 +177,18 @@ cdef class Representer:
cdef RepresenterState _handle_wait_value_ScalarEvent(self, object ev):
key = self.keys.pop()
- (<MappingNode> self.output[-1]).value[key] = \
- ScalarNode.__new__(ScalarNode, self._file_index, ev.start_mark.line, ev.start_mark.column, ev.value)
+ (<MappingNode> self.output[-1])._set(key,
+ ScalarNode.__new__(ScalarNode,
+ self._file_index,
+ ev.start_mark.line,
+ ev.start_mark.column,
+ ev.value))
return RepresenterState.wait_key
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]).value[key] = self.output[-1]
+ (<MappingNode> self.output[-2])._set(key, self.output[-1])
return new_state
cdef RepresenterState _handle_wait_key_MappingEndEvent(self, object ev):
@@ -202,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]).value[self.keys[-1]] = self.output[-1]
+ (<MappingNode> self.output[-2])._set(self.keys[-1], 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 c26d78a..769de9a 100644
--- a/src/buildstream/node.pxd
+++ b/src/buildstream/node.pxd
@@ -71,6 +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 Node _get(self, str key, default, default_constructor)
diff --git a/src/buildstream/node.pyx b/src/buildstream/node.pyx
index 3941383..42a0794 100644
--- a/src/buildstream/node.pyx
+++ b/src/buildstream/node.pyx
@@ -53,6 +53,7 @@ Class Reference
"""
import string
+import sys
from ._exceptions import LoadError
from .exceptions import LoadErrorReason
@@ -503,36 +504,7 @@ cdef class MappingNode(Node):
del self.value[key]
def __setitem__(self, str key, object value):
- cdef Node old_value
-
- if type(value) in [MappingNode, ScalarNode, SequenceNode]:
- self.value[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(key)
- if old_value:
- node.file_index = old_value.file_index
- node.line = old_value.line
- node.column = old_value.column
-
- self.value[key] = node
+ self._set(key, value)
#############################################################
# Public Methods #
@@ -1126,6 +1098,49 @@ cdef class MappingNode(Node):
target.line = self.line
target.column = self.column
+ # _set(key, value)
+ #
+ # Internal helper method to set an entry. This is used
+ # everywhere that a MappingNode value can be set, and ensures
+ # that MappingNode keys are always interned.
+ #
+ # Args:
+ # 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 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
+
# _get(key, default, default_constructor)
#
# Internal helper method to get an entry from the underlying dictionary.