[Notes] [Git][BuildStream/buildstream][danielsilverstone-ct/further-optimisations] 3 commits: _yaml.py: Remove use of isinstance() in `{node, list}_{chain_, }copy`

[Daniel Silverstone <gitlab mg gitlab com>]

Title: GitLab

Daniel Silverstone pushed to branch danielsilverstone-ct/further-optimisations at BuildStream / buildstream

Commits:

• bb965ba2
  by Daniel Silverstone at 2019-02-12T10:19:55Z

  _yaml.py: Remove use of isinstance() in `{node,list}_{chain_,}copy`
  
  A non-trivial proportion of time pre-scheduler in `bst build` is spent
  copying (or chain-copying) nodes.  Approximately a quarter of the time
  spent in that effort is in `isinstance()`.  This removes that CPU load.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• bf825da5
  by Daniel Silverstone at 2019-02-12T10:19:55Z

  _yaml.py: Reduce cost of node_final_assertions
  
  By re-using the isinstance replacements from earlier commits and
  using a tuple of the string constants for checking for composition
  markers, we reduce the cost of node_final_assertions by two thirds
  in basic testing.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• bd21bf2c
  by Daniel Silverstone at 2019-02-12T13:07:14Z

  _context.py: Cache result of get_strict()
  
  While get_strict() doesn't look expensive per-se, it is called so many
  times that it is valuable to cache the result once computed.  Since I
  don't think it can change once it is computable, cache it immediately
  that becomes possible and we save 20s in my test case.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

2 changed files:

• buildstream/_context.py
• buildstream/_yaml.py

Changes:

buildstream/_context.py

---

...	...	@@ -363,12 +363,15 @@ class Context():
363	363	def get_strict(self):
364	364
365	365	# If it was set by the CLI, it overrides any config
	366	+ # Ditto if we've already computed this, then we return the computed
	367	+ # value which we cache here too
366	368	if self._strict_build_plan is not None:
367	369	return self._strict_build_plan
368	370
369	371	toplevel = self.get_toplevel_project()
370	372	overrides = self.get_overrides(toplevel.name)
371		- return _yaml.node_get(overrides, bool, 'strict', default_value=True)
	373	+ self._strict_build_plan = _yaml.node_get(overrides, bool, 'strict', default_value=True)
	374	+ return self._strict_build_plan
372	375
373	376	# get_cache_key():
374	377	#

buildstream/_yaml.py

---

...	...	@@ -1075,15 +1075,48 @@ class ChainMap(collections.ChainMap):
1075	1075	return default
1076	1076
1077	1077
	1078	+# Node copying
	1079	+#
	1080	+# Unfortunately we copy nodes a *lot* and `isinstance()` is super-slow when
	1081	+# things from collections.abc get involved. The result is the following
	1082	+# intricate but substantially faster group of tuples and the use of `in`.
	1083	+#
	1084	+# If any of the {node,list}_{chain_,}_copy routines raise a ValueError
	1085	+# then it's likely additional types need adding to these tuples.
	1086	+
	1087	+# When chaining a copy, these types are skipped since the ChainMap will
	1088	+# retrieve them from the source node when needed. Other copiers might copy
	1089	+# them, so we call them __quick_types.
	1090	+__quick_types = (str, bool,
	1091	+ yaml.scalarstring.PreservedScalarString,
	1092	+ yaml.scalarstring.SingleQuotedScalarString,
	1093	+ yaml.scalarstring.DoubleQuotedScalarString)
	1094	+
	1095	+# These types have to be iterated like a dictionary
	1096	+__dict_types = (dict, ChainMap, yaml.comments.CommentedMap)
	1097	+
	1098	+# These types have to be iterated like a list
	1099	+__list_types = (list, yaml.comments.CommentedSeq)
	1100	+
	1101	+# These are the provenance types, which have to be cloned rather than any other
	1102	+# copying tactic.
	1103	+__provenance_types = (Provenance, DictProvenance, MemberProvenance, ElementProvenance)
	1104	+
	1105	+
1078	1106	def node_chain_copy(source):
1079	1107	copy = ChainMap({}, source)
1080	1108	for key, value in source.items():
1081		- if isinstance(value, collections.abc.Mapping):
	1109	+ value_type = type(value)
	1110	+ if value_type in __dict_types:
1082	1111	copy[key] = node_chain_copy(value)
1083		- elif isinstance(value, list):
	1112	+ elif value_type in __list_types:
1084	1113	copy[key] = list_chain_copy(value)
1085		- elif isinstance(value, Provenance):
	1114	+ elif value_type in __provenance_types:
1086	1115	copy[key] = value.clone()
	1116	+ elif value_type in __quick_types:
	1117	+ pass # No need to copy these, the chainmap deals with it
	1118	+ else:
	1119	+ raise ValueError("Unable to be quick about node_chain_copy of {}".format(value_type))
1087	1120
1088	1121	return copy
1089	1122
...	...	@@ -1091,14 +1124,17 @@ def node_chain_copy(source):
1091	1124	def list_chain_copy(source):
1092	1125	copy = []
1093	1126	for item in source:
1094		- if isinstance(item, collections.abc.Mapping):
	1127	+ item_type = type(item)
	1128	+ if item_type in __dict_types:
1095	1129	copy.append(node_chain_copy(item))
1096		- elif isinstance(item, list):
	1130	+ elif item_type in __list_types:
1097	1131	copy.append(list_chain_copy(item))
1098		- elif isinstance(item, Provenance):
	1132	+ elif item_type in __provenance_types:
1099	1133	copy.append(item.clone())
1100		- else:
	1134	+ elif item_type in __quick_types:
1101	1135	copy.append(item)
	1136	+ else: # Fallback
	1137	+ raise ValueError("Unable to be quick about list_chain_copy of {}".format(item_type))
1102	1138
1103	1139	return copy
1104	1140
...	...	@@ -1106,14 +1142,17 @@ def list_chain_copy(source):
1106	1142	def node_copy(source):
1107	1143	copy = {}
1108	1144	for key, value in source.items():
1109		- if isinstance(value, collections.abc.Mapping):
	1145	+ value_type = type(value)
	1146	+ if value_type in __dict_types:
1110	1147	copy[key] = node_copy(value)
1111		- elif isinstance(value, list):
	1148	+ elif value_type in __list_types:
1112	1149	copy[key] = list_copy(value)
1113		- elif isinstance(value, Provenance):
	1150	+ elif value_type in __provenance_types:
1114	1151	copy[key] = value.clone()
1115		- else:
	1152	+ elif value_type in __quick_types:
1116	1153	copy[key] = value
	1154	+ else:
	1155	+ raise ValueError("Unable to be quick about node_copy of {}".format(value_type))
1117	1156
1118	1157	ensure_provenance(copy)
1119	1158
...	...	@@ -1123,18 +1162,25 @@ def node_copy(source):
1123	1162	def list_copy(source):
1124	1163	copy = []
1125	1164	for item in source:
1126		- if isinstance(item, collections.abc.Mapping):
	1165	+ item_type = type(item)
	1166	+ if item_type in __dict_types:
1127	1167	copy.append(node_copy(item))
1128		- elif isinstance(item, list):
	1168	+ elif item_type in __list_types:
1129	1169	copy.append(list_copy(item))
1130		- elif isinstance(item, Provenance):
	1170	+ elif item_type in __provenance_types:
1131	1171	copy.append(item.clone())
1132		- else:
	1172	+ elif item_type in __quick_types:
1133	1173	copy.append(item)
	1174	+ else:
	1175	+ raise ValueError("Unable to be quick about list_copy of {}".format(item_type))
1134	1176
1135	1177	return copy
1136	1178
1137	1179
	1180	+# Helpers for the assertions
	1181	+__node_assert_composition_directives = ('(>)', '(<)', '(=)')
	1182	+
	1183	+
1138	1184	# node_final_assertions()
1139	1185	#
1140	1186	# This must be called on a fully loaded and composited node,
...	...	@@ -1153,22 +1199,26 @@ def node_final_assertions(node):
1153	1199	# indicates that the user intended to override a list which
1154	1200	# never existed in the underlying data
1155	1201	#
1156		- if key in ['(>)', '(<)', '(=)']:
	1202	+ if key in __node_assert_composition_directives:
1157	1203	provenance = node_get_provenance(node, key)
1158	1204	raise LoadError(LoadErrorReason.TRAILING_LIST_DIRECTIVE,
1159	1205	"{}: Attempt to override non-existing list".format(provenance))
1160	1206
1161		- if isinstance(value, collections.abc.Mapping):
	1207	+ value_type = type(value)
	1208	+
	1209	+ if value_type in __dict_types:
1162	1210	node_final_assertions(value)
1163		- elif isinstance(value, list):
	1211	+ elif value_type in __list_types:
1164	1212	list_final_assertions(value)
1165	1213
1166	1214
1167	1215	def list_final_assertions(values):
1168	1216	for value in values:
1169		- if isinstance(value, collections.abc.Mapping):
	1217	+ value_type = type(value)
	1218	+
	1219	+ if value_type in __dict_types:
1170	1220	node_final_assertions(value)
1171		- elif isinstance(value, list):
	1221	+ elif value_type in __list_types:
1172	1222	list_final_assertions(value)
1173	1223
1174	1224

—
View it on GitLab.
You're receiving this email because of your account on gitlab.com. If you'd like to receive fewer emails, you can adjust your notification settings.