[Notes] [Git][BuildStream/buildstream][danielsilverstone-ct/further-optimisations] 4 commits: _yaml.py: Reduce use of `isinstance()` in `node_sanitize()`

[Daniel Silverstone <gitlab mg gitlab com>]

Title: GitLab

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

Commits:

• 3cbb4d78
  by Daniel Silverstone at 2019-02-12T13:56:55Z

  _yaml.py: Reduce use of `isinstance()` in `node_sanitize()`
  
  We know that nodes are typically one of:
  str, list, dict, bool, tuple, NoneType or our ChainMap
  Of these, dict and ChainMap are Mapping, only list is list
  and the rest are returned unchanged.  We can reduce/defer our use
  of isinstance here, dramatically, improving performance.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• 14676258
  by Daniel Silverstone at 2019-02-12T13:56: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>
  

• 8d1f4fc5
  by Daniel Silverstone at 2019-02-12T14:02:32Z

  _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>
  

• 02be7f74
  by Daniel Silverstone at 2019-02-12T14:04:39Z

  _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

---

...	...	@@ -361,14 +361,17 @@ class Context():
361	361	# (bool): Whether or not to use strict build plan
362	362	#
363	363	def get_strict(self):
	364	+ if self._strict_build_plan is None:
	365	+ # Either we're not overridden or we've never worked it out before
	366	+ # so work out if we should be strict, and then cache the result
	367	+ toplevel = self.get_toplevel_project()
	368	+ overrides = self.get_overrides(toplevel.name)
	369	+ self._strict_build_plan = _yaml.node_get(overrides, bool, 'strict', default_value=True)
364	370
365	371	# If it was set by the CLI, it overrides any config
366		- if self._strict_build_plan is not None:
367		- return self._strict_build_plan
368		-
369		- toplevel = self.get_toplevel_project()
370		- overrides = self.get_overrides(toplevel.name)
371		- return _yaml.node_get(overrides, bool, 'strict', default_value=True)
	372	+ # Ditto if we've already computed this, then we return the computed
	373	+ # value which we cache here too.
	374	+ return self._strict_build_plan
372	375
373	376	# get_cache_key():
374	377	#

buildstream/_yaml.py

---

...	...	@@ -914,6 +914,10 @@ RoundTripRepresenter.add_representer(SanitizedDict,
914	914	SafeRepresenter.represent_dict)
915	915
916	916
	917	+# Types we can short-circuit in node_sanitize for speed.
	918	+__sanitize_short_circuit_types = (int, float, str, bool, tuple)
	919	+
	920	+
917	921	# node_sanitize()
918	922	#
919	923	# Returnes an alphabetically ordered recursive copy
...	...	@@ -922,9 +926,21 @@ RoundTripRepresenter.add_representer(SanitizedDict,
922	926	# Only dicts are ordered, list elements are left in order.
923	927	#
924	928	def node_sanitize(node):
	929	+ # Short-circuit None which occurs ca. twice per element
	930	+ if node is None:
	931	+ return node
	932	+
	933	+ node_type = type(node)
	934	+ # Next short-circuit integers, floats, strings, booleans, and tuples
	935	+ if node_type in __sanitize_short_circuit_types:
	936	+ return node
	937	+ # Now short-circuit lists. Note this is only for the raw list
	938	+ # type, CommentedSeq and others get caught later.
	939	+ elif node_type is list:
	940	+ return [node_sanitize(elt) for elt in node]
925	941
926		- if isinstance(node, collections.abc.Mapping):
927		-
	942	+ # Finally ChainMap and dict, and other Mappings need special handling
	943	+ if node_type in (dict, ChainMap) or isinstance(node, collections.Mapping):
928	944	result = SanitizedDict()
929	945
930	946	key_list = [key for key, _ in node_items(node)]
...	...	@@ -932,10 +948,12 @@ def node_sanitize(node):
932	948	result[key] = node_sanitize(node[key])
933	949
934	950	return result
935		-
	951	+ # Catch the case of CommentedSeq and friends. This is more rare and so
	952	+ # we keep complexity down by still using isinstance here.
936	953	elif isinstance(node, list):
937	954	return [node_sanitize(elt) for elt in node]
938	955
	956	+ # Everything else (such as commented scalars) just gets returned as-is.
939	957	return node
940	958
941	959
...	...	@@ -1064,15 +1082,48 @@ class ChainMap(collections.ChainMap):
1064	1082	return default
1065	1083
1066	1084
	1085	+# Node copying
	1086	+#
	1087	+# Unfortunately we copy nodes a *lot* and `isinstance()` is super-slow when
	1088	+# things from collections.abc get involved. The result is the following
	1089	+# intricate but substantially faster group of tuples and the use of `in`.
	1090	+#
	1091	+# If any of the {node,list}_{chain_,}_copy routines raise a ValueError
	1092	+# then it's likely additional types need adding to these tuples.
	1093	+
	1094	+# When chaining a copy, these types are skipped since the ChainMap will
	1095	+# retrieve them from the source node when needed. Other copiers might copy
	1096	+# them, so we call them __quick_types.
	1097	+__quick_types = (str, bool,
	1098	+ yaml.scalarstring.PreservedScalarString,
	1099	+ yaml.scalarstring.SingleQuotedScalarString,
	1100	+ yaml.scalarstring.DoubleQuotedScalarString)
	1101	+
	1102	+# These types have to be iterated like a dictionary
	1103	+__dict_types = (dict, ChainMap, yaml.comments.CommentedMap)
	1104	+
	1105	+# These types have to be iterated like a list
	1106	+__list_types = (list, yaml.comments.CommentedSeq)
	1107	+
	1108	+# These are the provenance types, which have to be cloned rather than any other
	1109	+# copying tactic.
	1110	+__provenance_types = (Provenance, DictProvenance, MemberProvenance, ElementProvenance)
	1111	+
	1112	+
1067	1113	def node_chain_copy(source):
1068	1114	copy = ChainMap({}, source)
1069	1115	for key, value in source.items():
1070		- if isinstance(value, collections.abc.Mapping):
	1116	+ value_type = type(value)
	1117	+ if value_type in __dict_types:
1071	1118	copy[key] = node_chain_copy(value)
1072		- elif isinstance(value, list):
	1119	+ elif value_type in __list_types:
1073	1120	copy[key] = list_chain_copy(value)
1074		- elif isinstance(value, Provenance):
	1121	+ elif value_type in __provenance_types:
1075	1122	copy[key] = value.clone()
	1123	+ elif value_type in __quick_types:
	1124	+ pass # No need to copy these, the chainmap deals with it
	1125	+ else:
	1126	+ raise ValueError("Unable to be quick about node_chain_copy of {}".format(value_type))
1076	1127
1077	1128	return copy
1078	1129
...	...	@@ -1080,14 +1131,17 @@ def node_chain_copy(source):
1080	1131	def list_chain_copy(source):
1081	1132	copy = []
1082	1133	for item in source:
1083		- if isinstance(item, collections.abc.Mapping):
	1134	+ item_type = type(item)
	1135	+ if item_type in __dict_types:
1084	1136	copy.append(node_chain_copy(item))
1085		- elif isinstance(item, list):
	1137	+ elif item_type in __list_types:
1086	1138	copy.append(list_chain_copy(item))
1087		- elif isinstance(item, Provenance):
	1139	+ elif item_type in __provenance_types:
1088	1140	copy.append(item.clone())
1089		- else:
	1141	+ elif item_type in __quick_types:
1090	1142	copy.append(item)
	1143	+ else: # Fallback
	1144	+ raise ValueError("Unable to be quick about list_chain_copy of {}".format(item_type))
1091	1145
1092	1146	return copy
1093	1147
...	...	@@ -1095,14 +1149,17 @@ def list_chain_copy(source):
1095	1149	def node_copy(source):
1096	1150	copy = {}
1097	1151	for key, value in source.items():
1098		- if isinstance(value, collections.abc.Mapping):
	1152	+ value_type = type(value)
	1153	+ if value_type in __dict_types:
1099	1154	copy[key] = node_copy(value)
1100		- elif isinstance(value, list):
	1155	+ elif value_type in __list_types:
1101	1156	copy[key] = list_copy(value)
1102		- elif isinstance(value, Provenance):
	1157	+ elif value_type in __provenance_types:
1103	1158	copy[key] = value.clone()
1104		- else:
	1159	+ elif value_type in __quick_types:
1105	1160	copy[key] = value
	1161	+ else:
	1162	+ raise ValueError("Unable to be quick about node_copy of {}".format(value_type))
1106	1163
1107	1164	ensure_provenance(copy)
1108	1165
...	...	@@ -1112,18 +1169,25 @@ def node_copy(source):
1112	1169	def list_copy(source):
1113	1170	copy = []
1114	1171	for item in source:
1115		- if isinstance(item, collections.abc.Mapping):
	1172	+ item_type = type(item)
	1173	+ if item_type in __dict_types:
1116	1174	copy.append(node_copy(item))
1117		- elif isinstance(item, list):
	1175	+ elif item_type in __list_types:
1118	1176	copy.append(list_copy(item))
1119		- elif isinstance(item, Provenance):
	1177	+ elif item_type in __provenance_types:
1120	1178	copy.append(item.clone())
1121		- else:
	1179	+ elif item_type in __quick_types:
1122	1180	copy.append(item)
	1181	+ else:
	1182	+ raise ValueError("Unable to be quick about list_copy of {}".format(item_type))
1123	1183
1124	1184	return copy
1125	1185
1126	1186
	1187	+# Helpers for the assertions
	1188	+__node_assert_composition_directives = ('(>)', '(<)', '(=)')
	1189	+
	1190	+
1127	1191	# node_final_assertions()
1128	1192	#
1129	1193	# This must be called on a fully loaded and composited node,
...	...	@@ -1142,22 +1206,26 @@ def node_final_assertions(node):
1142	1206	# indicates that the user intended to override a list which
1143	1207	# never existed in the underlying data
1144	1208	#
1145		- if key in ['(>)', '(<)', '(=)']:
	1209	+ if key in __node_assert_composition_directives:
1146	1210	provenance = node_get_provenance(node, key)
1147	1211	raise LoadError(LoadErrorReason.TRAILING_LIST_DIRECTIVE,
1148	1212	"{}: Attempt to override non-existing list".format(provenance))
1149	1213
1150		- if isinstance(value, collections.abc.Mapping):
	1214	+ value_type = type(value)
	1215	+
	1216	+ if value_type in __dict_types:
1151	1217	node_final_assertions(value)
1152		- elif isinstance(value, list):
	1218	+ elif value_type in __list_types:
1153	1219	list_final_assertions(value)
1154	1220
1155	1221
1156	1222	def list_final_assertions(values):
1157	1223	for value in values:
1158		- if isinstance(value, collections.abc.Mapping):
	1224	+ value_type = type(value)
	1225	+
	1226	+ if value_type in __dict_types:
1159	1227	node_final_assertions(value)
1160		- elif isinstance(value, list):
	1228	+ elif value_type in __list_types:
1161	1229	list_final_assertions(value)
1162	1230
1163	1231

—
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.