[Notes] [Git][BuildStream/buildstream][master] 6 commits: _yaml.py: Only retrieve provenance in node_get() when needed

[Benjamin Schubert <gitlab mg gitlab com>]

Title: GitLab

Benjamin Schubert pushed to branch master at BuildStream / buildstream

Commits:

• a39c4767
  by Daniel Silverstone at 2019-02-12T15:12:30Z

  _yaml.py: Only retrieve provenance in node_get() when needed
  
  We were indiscriminately retrieving the node's provenance data in the
  `node_get()` function which was accounting for approximately a third of
  the total runtime of `node_get()` which dominates pre-scheduler time in
  `bst build`.  This change ameliorates that situation by only retrieving
  the provenance data when it's actually needed.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• cda03313
  by Daniel Silverstone at 2019-02-12T15:12:30Z

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

• b6f08e1b
  by Daniel Silverstone at 2019-02-12T15:12:30Z

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

• 69ee11c6
  by Daniel Silverstone at 2019-02-12T15:12:30Z

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

• 0928e570
  by Daniel Silverstone at 2019-02-12T15:12:30Z

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

• 022a59f0
  by Benjamin Schubert at 2019-02-12T16:14:05Z

  Merge branch 'danielsilverstone-ct/further-optimisations' into 'master'
  
  Further optimisations
  
  See merge request BuildStream/buildstream!1131

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

---

...	...	@@ -365,8 +365,8 @@ _sentinel = object()
365	365	#
366	366	def node_get(node, expected_type, key, indices=None, *, default_value=_sentinel, allow_none=False):
367	367	value = node.get(key, default_value)
368		- provenance = node_get_provenance(node)
369	368	if value is _sentinel:
	369	+ provenance = node_get_provenance(node)
370	370	raise LoadError(LoadErrorReason.INVALID_DATA,
371	371	"{}: Dictionary did not contain expected key '{}'".format(provenance, key))
372	372
...	...	@@ -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,52 @@ 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	+# These are the directives used to compose lists, we need this because it's
	1113	+# slightly faster during the node_final_assertions checks
	1114	+__NODE_ASSERT_COMPOSITION_DIRECTIVES = ('(>)', '(<)', '(=)')
	1115	+
	1116	+
1067	1117	def node_chain_copy(source):
1068	1118	copy = ChainMap({}, source)
1069	1119	for key, value in source.items():
1070		- if isinstance(value, collections.abc.Mapping):
	1120	+ value_type = type(value)
	1121	+ if value_type in __DICT_TYPES:
1071	1122	copy[key] = node_chain_copy(value)
1072		- elif isinstance(value, list):
	1123	+ elif value_type in __LIST_TYPES:
1073	1124	copy[key] = list_chain_copy(value)
1074		- elif isinstance(value, Provenance):
	1125	+ elif value_type in __PROVENANCE_TYPES:
1075	1126	copy[key] = value.clone()
	1127	+ elif value_type in __QUICK_TYPES:
	1128	+ pass # No need to copy these, the chainmap deals with it
	1129	+ else:
	1130	+ raise ValueError("Unable to be quick about node_chain_copy of {}".format(value_type))
1076	1131
1077	1132	return copy
1078	1133
...	...	@@ -1080,14 +1135,17 @@ def node_chain_copy(source):
1080	1135	def list_chain_copy(source):
1081	1136	copy = []
1082	1137	for item in source:
1083		- if isinstance(item, collections.abc.Mapping):
	1138	+ item_type = type(item)
	1139	+ if item_type in __DICT_TYPES:
1084	1140	copy.append(node_chain_copy(item))
1085		- elif isinstance(item, list):
	1141	+ elif item_type in __LIST_TYPES:
1086	1142	copy.append(list_chain_copy(item))
1087		- elif isinstance(item, Provenance):
	1143	+ elif item_type in __PROVENANCE_TYPES:
1088	1144	copy.append(item.clone())
1089		- else:
	1145	+ elif item_type in __QUICK_TYPES:
1090	1146	copy.append(item)
	1147	+ else: # Fallback
	1148	+ raise ValueError("Unable to be quick about list_chain_copy of {}".format(item_type))
1091	1149
1092	1150	return copy
1093	1151
...	...	@@ -1095,14 +1153,17 @@ def list_chain_copy(source):
1095	1153	def node_copy(source):
1096	1154	copy = {}
1097	1155	for key, value in source.items():
1098		- if isinstance(value, collections.abc.Mapping):
	1156	+ value_type = type(value)
	1157	+ if value_type in __DICT_TYPES:
1099	1158	copy[key] = node_copy(value)
1100		- elif isinstance(value, list):
	1159	+ elif value_type in __LIST_TYPES:
1101	1160	copy[key] = list_copy(value)
1102		- elif isinstance(value, Provenance):
	1161	+ elif value_type in __PROVENANCE_TYPES:
1103	1162	copy[key] = value.clone()
1104		- else:
	1163	+ elif value_type in __QUICK_TYPES:
1105	1164	copy[key] = value
	1165	+ else:
	1166	+ raise ValueError("Unable to be quick about node_copy of {}".format(value_type))
1106	1167
1107	1168	ensure_provenance(copy)
1108	1169
...	...	@@ -1112,14 +1173,17 @@ def node_copy(source):
1112	1173	def list_copy(source):
1113	1174	copy = []
1114	1175	for item in source:
1115		- if isinstance(item, collections.abc.Mapping):
	1176	+ item_type = type(item)
	1177	+ if item_type in __DICT_TYPES:
1116	1178	copy.append(node_copy(item))
1117		- elif isinstance(item, list):
	1179	+ elif item_type in __LIST_TYPES:
1118	1180	copy.append(list_copy(item))
1119		- elif isinstance(item, Provenance):
	1181	+ elif item_type in __PROVENANCE_TYPES:
1120	1182	copy.append(item.clone())
1121		- else:
	1183	+ elif item_type in __QUICK_TYPES:
1122	1184	copy.append(item)
	1185	+ else:
	1186	+ raise ValueError("Unable to be quick about list_copy of {}".format(item_type))
1123	1187
1124	1188	return copy
1125	1189
...	...	@@ -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.