From fa49c34ac1f03410a74a8508497fa94f3009ccb2 Mon Sep 17 00:00:00 2001 From: Ville Laitila Date: Fri, 11 Sep 2026 17:30:54 +0300 Subject: [PATCH 1/2] XML reader: pool repeated attribute strings instead of one object per occurrence expat hands out a fresh str object for every attribute name and value it reports, and sgraph models repeat both heavily. A 250k-element model carries 2.4M attribute occurrences drawn from 181 distinct names and ~90k distinct values, so its attribute names and values alone retained 2,831,679 string objects where 90,019 suffice. SGraphXMLParser now keeps a per-parse dict and routes attribute names, attribute values, element types and association dependency types through it, collapsing equal strings onto one object. Measured on three real models (retained RSS after parse, best of three runs): openclaw 627 MB -> 514 MB (-18%) intra 535 MB -> 429 MB (-20%) odoo-fullstack 473 MB -> 400 MB (-15%) Counted exactly rather than via RSS, the strings the openclaw model retains (element names, attribute names and values, dependency types) drop from 3,235,297 objects / 213.2 MB to 339,472 objects / 54.6 MB. Attribute-name objects alone go from 2,158,742 to 181. Parse time is unchanged within run-to-run noise. sys.intern() would collapse the same strings just as well - measured over this model it retains the identical 54.6 MB, and across two models loaded at once it shares only 0.2 MB more than this pool does. It is not used because it mutates interpreter-global state from a hot parsing loop for no measured gain, and because it raises TypeError on the None that an attribute written as legitimately produces. A dict on the parser instance keeps both the mechanism and the strings' lifetime local to the reader. The saving is a property of the data, not a guarantee: a synthetic model in which no name and no value repeats gains nothing and pays about 5% more RSS growth during the parse. Real models never look like that, because analyzers draw attribute names from a fixed vocabulary. Verified beyond the new tests: canonical dumps of every element path, its attributes, and every association's endpoints, deptype and attributes are identical to the pre-change reader on two 250k-element models, and to_deps output is byte-identical. Claude-Session: https://claude.ai/code/session_01XqfUmwZ3VYazHpKAFnYDFF --- src/sgraph/sgraph.py | 49 +++++++-- tests/test_parse_string_pooling.py | 160 +++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+), 6 deletions(-) create mode 100644 tests/test_parse_string_pooling.py diff --git a/src/sgraph/sgraph.py b/src/sgraph/sgraph.py index 443053a..55eb5f9 100644 --- a/src/sgraph/sgraph.py +++ b/src/sgraph/sgraph.py @@ -646,6 +646,38 @@ def __init__(self): self.blacklisted_assoc_attributes: set[str] = set() self.ignore_all_assoc_attributes = False + # Attribute names, attribute values, element types and dependency types + # repeat heavily across a model, but expat hands out a fresh str object per + # occurrence. A measured 250k-element model carries 2.4M attribute + # occurrences drawn from 181 distinct names and ~90k distinct values, so its + # attribute names and values alone retained 2.83M string objects where 90k + # suffice. Pooling collapses those onto one object each: ~80% off the + # model's attribute-string bytes and 15-20% off its total footprint, at a + # parse cost inside run-to-run noise. The saving is a property of the data, + # not a guarantee: a synthetic model in which no name and no value repeats + # gains nothing and pays ~5% more RSS growth during the parse for a pool it + # cannot use. Real models never look like that - the analyzers draw + # attribute names from a fixed vocabulary - so names pool even when values + # do not. + # sys.intern() would collapse the same strings just as well: measured over + # this model it retains the identical 54.6 MB, and across two models loaded + # at once it shares only 0.2 MB more than this pool does. It is not used + # because it mutates interpreter-global state from a hot parsing loop for no + # measured gain, and because it raises TypeError on the None that an + # attribute written as legitimately produces. A dict on the + # parser instance keeps both the mechanism and the strings' lifetime local + # to the reader: the pool is discarded when parsing finishes, leaving the + # pooled strings reachable only through the model itself. + self._string_pool: dict[str | None, str | None] = {} + + def _shared(self, value: str | None) -> str | None: + """Return the pool's canonical object for an equal string. + + An attribute written as carries no value at all, so None + reaches this too and passes straight through, as it did before pooling. + """ + return self._string_pool.setdefault(value, value) + def set_type_rules(self, the_type_rules: Optional[list[str]]): if the_type_rules is None: self.acceptableAssocTypes = None @@ -713,7 +745,7 @@ def startElement(self, tag_name: str, attrs: AttributesImpl): return value = attrs.get('v') - self.currentRelation[name] = value # type: ignore + self.currentRelation[self._shared(name)] = self._shared(value) else: if self.currentElement is not None and len(self.currentElementPath) > 0: if name in self.blacklisted_elem_attributes: @@ -724,7 +756,8 @@ def startElement(self, tag_name: str, attrs: AttributesImpl): self.property += 1 value = attrs.get('v') - self.currentElement.addAttribute(name, value) # type: ignore + self.currentElement.addAttribute(self._shared(name), + self._shared(value)) else: val = attrs.get('v') sys.stderr.write(f' discarding {name} {val} attrs, no element to assign the data\n') @@ -745,7 +778,7 @@ def startElement(self, tag_name: str, attrs: AttributesImpl): for aname, avalue in list(attrs.items()): if aname == 't' or aname == 'type': - e.setType(avalue) + e.setType(self._shared(avalue)) self.property += 1 elif aname == 'i': self.id_to_elem_map[avalue] = e @@ -754,9 +787,11 @@ def startElement(self, tag_name: str, attrs: AttributesImpl): if not aname in self.blacklisted_elem_attributes: if self.whitelisted_elem_attributes: if aname in self.whitelisted_elem_attributes: - e.addAttribute(aname, avalue) + e.addAttribute(self._shared(aname), + self._shared(avalue)) else: - e.addAttribute(aname, avalue) + e.addAttribute(self._shared(aname), + self._shared(avalue)) if self.only_root: @@ -766,6 +801,8 @@ def startElement(self, tag_name: str, attrs: AttributesImpl): self.currentRelation = {} referred = attrs.get('r') t = attrs.get('t') + if t is not None: + t = self._shared(t) redirectEnabled = False if not redirectEnabled: self.link += 1 @@ -779,7 +816,7 @@ def startElement(self, tag_name: str, attrs: AttributesImpl): for aname, avalue in list(attrs.items()): if len(aname) > 1: - self.currentRelation[aname] = avalue + self.currentRelation[self._shared(aname)] = self._shared(avalue) def endElement(self, name: str): if name == 'e': diff --git a/tests/test_parse_string_pooling.py b/tests/test_parse_string_pooling.py new file mode 100644 index 0000000..b07dddb --- /dev/null +++ b/tests/test_parse_string_pooling.py @@ -0,0 +1,160 @@ +"""The XML reader must hand out one string object per distinct attribute name, +attribute value, element type and dependency type, instead of a fresh object per +occurrence. Models repeat these heavily, so the duplicates dominate the loaded +model's string footprint. + +These tests assert object identity because it is the only cheap proxy for "this +string is stored once". Identity is not an sgraph guarantee: nothing in the +library compares attribute strings with `is`, and nothing may start to. Compare +attribute values with `==`.""" +import io + +from sgraph import SGraph + +# Two sibling files carrying the same attribute names and the same values, plus a +# dependency between them. Values are multi-character so that CPython's own +# caching of latin-1 singletons cannot be mistaken for pooling. +MODEL = """ + + + + + + + + + + + + + + + + +""" + + +# Same shape, but the association attributes are written as XML attributes on . +INLINE_ASSOC_MODEL = """ + + + + + + + + + + +""" + + +def parse(): + return SGraph.parse_xml_file_or_stream(io.StringIO(MODEL)) + + +def files_of(graph): + repo = graph.rootNode.children[0] + return {child.name: child for child in repo.children} + + +def test_repeated_attribute_names_share_one_object(): + files = files_of(parse()) + name_a = next(k for k in files['a.py'].attrs if k == 'analyzer_name') + name_b = next(k for k in files['b.py'].attrs if k == 'analyzer_name') + + assert name_a is name_b + + +def test_repeated_attribute_values_share_one_object(): + files = files_of(parse()) + + assert files['a.py'].attrs['analyzer_name'] is files['b.py'].attrs['analyzer_name'] + + +def test_repeated_inline_attribute_values_share_one_object(): + """Element attributes written as XML attributes on , not as children.""" + files = files_of(parse()) + + assert files['a.py'].attrs['language'] is files['b.py'].attrs['language'] + + +def test_repeated_inline_attribute_names_share_one_object(): + """expat does not reuse attribute-name objects across a document, so the inline + form needs pooling just as much as the form does.""" + files = files_of(parse()) + name_a = next(k for k in files['a.py'].attrs if k == 'language') + name_b = next(k for k in files['b.py'].attrs if k == 'language') + + assert name_a is name_b + + +def test_repeated_inline_association_attribute_names_share_one_object(): + """Association attributes written as XML attributes on .""" + graph = SGraph.parse_xml_file_or_stream(io.StringIO(INLINE_ASSOC_MODEL)) + files = files_of(graph) + attrs_a = files['a.py'].outgoing[0].attrs + attrs_b = files['b.py'].outgoing[0].attrs + + name_a = next(k for k in attrs_a if k == 'call_context') + name_b = next(k for k in attrs_b if k == 'call_context') + assert name_a is name_b + assert attrs_a['call_context'] is attrs_b['call_context'] + + +def test_repeated_element_types_share_one_object(): + files = files_of(parse()) + + assert files['a.py'].typeEquals(files['b.py'].getType()) + assert files['a.py'].attrs['type'] is files['b.py'].attrs['type'] + + +def test_repeated_dependency_types_share_one_object(): + files = files_of(parse()) + dep_a = files['a.py'].outgoing[0] + dep_b = files['b.py'].outgoing[0] + + assert dep_a.deptype == 'function_ref' + assert dep_a.deptype is dep_b.deptype + + +def test_repeated_association_attributes_share_one_object(): + files = files_of(parse()) + attrs_a = files['a.py'].outgoing[0].attrs + attrs_b = files['b.py'].outgoing[0].attrs + + name_a = next(k for k in attrs_a if k == 'call_context') + name_b = next(k for k in attrs_b if k == 'call_context') + assert name_a is name_b + assert attrs_a['call_context'] is attrs_b['call_context'] + + +def test_whitelisted_inline_attributes_are_pooled(): + """The whitelist branch of the inline-element-attribute path pools too.""" + graph = SGraph.parse_xml_file_or_stream(io.StringIO(MODEL), + elem_attribute_filters=['language']) + files = files_of(graph) + + assert 'language' in files['a.py'].attrs + name_a = next(k for k in files['a.py'].attrs if k == 'language') + name_b = next(k for k in files['b.py'].attrs if k == 'language') + assert name_a is name_b + assert files['a.py'].attrs['language'] is files['b.py'].attrs['language'] + + +def test_pooling_does_not_outlive_the_parse(): + """The pool must not be process-global: two parses produce independent objects, + so a long-lived process that loads many models can free each model's strings.""" + first = files_of(parse())['a.py'].attrs['analyzer_name'] + second = files_of(parse())['a.py'].attrs['analyzer_name'] + + assert first == second + assert first is not second + + +def test_attribute_without_value_still_parses_as_none(): + xml = '' + graph = SGraph.parse_xml_file_or_stream(io.StringIO(xml)) + elem = graph.rootNode.children[0].children[0] + + assert elem.attrs['novalue'] is None From 8a32290f3f9394be5d0119f6bbe4e3b94390ee3a Mon Sep 17 00:00:00 2001 From: Ville Laitila Date: Fri, 11 Sep 2026 18:09:16 +0300 Subject: [PATCH 2/2] XML reader: make attribute filters reach every attribute they name elem_attribute_filters and assoc_attribute_filters were only partly wired up, in two independent ways. The handler returned early only when BOTH ignore-all flags were set, so passing `IGNORE *` for one kind of attribute alone did nothing to the spelling. `assoc_attribute_filters=['IGNORE *']` kept every association attribute; `elem_attribute_filters=['IGNORE *']` kept every element attribute written as an child. Separately, association attributes written inline on the tag went through a loop that consulted no filter at all, so no assoc filter - ignore-all, blacklist or whitelist - ever reached that spelling. The equivalent loop for already applied element filters; this brings in line with it. Each kind of filter now governs its own kind of attribute, in both spellings. On a 250k-element model, `IGNORE *` on both filter lists now drops the 231,927 association attributes it previously kept, taking the loaded model from 465 MB to 426 MB. Structural values are deliberately left alone, as before: an element's type and an association's deptype describe what the thing is rather than data attached to it, and no attribute filter reaches them. No behaviour changes for a load that passes no filters - the canonical dump of two 250k-element models is identical to before. There were no tests for attribute filters at all, which is why this survived; tests/test_parse_attribute_filters.py now covers both filter kinds against both spellings, and 5 of its 10 tests fail without this change. Claude-Session: https://claude.ai/code/session_01XqfUmwZ3VYazHpKAFnYDFF --- src/sgraph/sgraph.py | 17 ++++- tests/test_parse_attribute_filters.py | 106 ++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 tests/test_parse_attribute_filters.py diff --git a/src/sgraph/sgraph.py b/src/sgraph/sgraph.py index 55eb5f9..e5134f8 100644 --- a/src/sgraph/sgraph.py +++ b/src/sgraph/sgraph.py @@ -738,6 +738,8 @@ def startElement(self, tag_name: str, attrs: AttributesImpl): name = attrs.get('n') if self.currentRelation is not None: + if self.ignore_all_assoc_attributes: + return if name in self.blacklisted_assoc_attributes: return if self.whitelisted_assoc_attributes: @@ -748,6 +750,8 @@ def startElement(self, tag_name: str, attrs: AttributesImpl): self.currentRelation[self._shared(name)] = self._shared(value) else: if self.currentElement is not None and len(self.currentElementPath) > 0: + if self.ignore_all_elem_attributes: + return if name in self.blacklisted_elem_attributes: return if self.whitelisted_elem_attributes: @@ -814,9 +818,20 @@ def startElement(self, tag_name: str, attrs: AttributesImpl): elif referred is not None: self.createReference(referred, t) + # 'r' and 't' carry the reference and the dependency type, not user + # attributes, and are excluded by the length test. The rest are + # association attributes and obey the same filters as children do. for aname, avalue in list(attrs.items()): if len(aname) > 1: - self.currentRelation[self._shared(aname)] = self._shared(avalue) + if not self.ignore_all_assoc_attributes: + if aname not in self.blacklisted_assoc_attributes: + if self.whitelisted_assoc_attributes: + if aname in self.whitelisted_assoc_attributes: + self.currentRelation[self._shared(aname)] = \ + self._shared(avalue) + else: + self.currentRelation[self._shared(aname)] = \ + self._shared(avalue) def endElement(self, name: str): if name == 'e': diff --git a/tests/test_parse_attribute_filters.py b/tests/test_parse_attribute_filters.py new file mode 100644 index 0000000..3fbbedc --- /dev/null +++ b/tests/test_parse_attribute_filters.py @@ -0,0 +1,106 @@ +"""elem_attribute_filters and assoc_attribute_filters must each govern their own +kind of attribute, independently, and must reach both spellings the XML format +allows: an child, and an attribute written inline on the +enclosing or tag.""" +import io + +from sgraph import SGraph + +# Every element and every association carries the same two attributes, once in +# each spelling, so a filter that reaches only one spelling is visible. +MODEL = """ + + + + + + + + + + + + +""" + +ALL_ELEM = {'elem_inline': 'ei', 'elem_child': 'ec'} +ALL_ASSOC = {'assoc_inline': 'ai', 'assoc_child': 'ac'} + + +def load(**kwargs): + """Element and association attributes of /repo/a.py, minus the structural ones. + + 'type' on an element and deptype on an association describe what the thing is, + not data attached to it, so no attribute filter is meant to reach them - see + test_structural_type_survives_every_filter. + """ + graph = SGraph.parse_xml_file_or_stream(io.StringIO(MODEL), **kwargs) + repo = graph.rootNode.children[0] + elem = next(c for c in repo.children if c.name == 'a.py') + elem_attrs = {k: v for k, v in elem.attrs.items() if k != 'type'} + return elem_attrs, dict(elem.outgoing[0].attrs or {}) + + +def test_without_filters_every_attribute_is_kept(): + assert load() == (ALL_ELEM, ALL_ASSOC) + + +def test_assoc_ignore_all_drops_association_attributes_in_both_spellings(): + elem_attrs, assoc_attrs = load(assoc_attribute_filters=['IGNORE *']) + + assert assoc_attrs == {} + assert elem_attrs == ALL_ELEM, 'association filters must not touch element attributes' + + +def test_elem_ignore_all_drops_element_attributes_in_both_spellings(): + elem_attrs, assoc_attrs = load(elem_attribute_filters=['IGNORE *']) + + assert elem_attrs == {} + assert assoc_attrs == ALL_ASSOC, 'element filters must not touch association attributes' + + +def test_ignoring_everything_drops_everything(): + assert load(elem_attribute_filters=['IGNORE *'], + assoc_attribute_filters=['IGNORE *']) == ({}, {}) + + +def test_assoc_blacklist_reaches_both_spellings(): + assert load(assoc_attribute_filters=['IGNORE assoc_inline'])[1] == {'assoc_child': 'ac'} + assert load(assoc_attribute_filters=['IGNORE assoc_child'])[1] == {'assoc_inline': 'ai'} + + +def test_assoc_whitelist_reaches_both_spellings(): + assert load(assoc_attribute_filters=['assoc_inline'])[1] == {'assoc_inline': 'ai'} + assert load(assoc_attribute_filters=['assoc_child'])[1] == {'assoc_child': 'ac'} + + +def test_elem_blacklist_reaches_both_spellings(): + assert load(elem_attribute_filters=['IGNORE elem_inline'])[0] == {'elem_child': 'ec'} + assert load(elem_attribute_filters=['IGNORE elem_child'])[0] == {'elem_inline': 'ei'} + + +def test_elem_whitelist_reaches_both_spellings(): + assert load(elem_attribute_filters=['elem_inline'])[0] == {'elem_inline': 'ei'} + assert load(elem_attribute_filters=['elem_child'])[0] == {'elem_child': 'ec'} + + +def test_structural_type_survives_every_filter(): + """An element's type is set outside the attribute-filter path on purpose.""" + graph = SGraph.parse_xml_file_or_stream(io.StringIO(MODEL), + elem_attribute_filters=['IGNORE *'], + assoc_attribute_filters=['IGNORE *']) + repo = graph.rootNode.children[0] + elem = next(c for c in repo.children if c.name == 'a.py') + + assert elem.attrs == {'type': 'file'} + + +def test_filtering_association_attributes_keeps_the_association_itself(): + graph = SGraph.parse_xml_file_or_stream(io.StringIO(MODEL), + assoc_attribute_filters=['IGNORE *']) + repo = graph.rootNode.children[0] + elem = next(c for c in repo.children if c.name == 'a.py') + + assert len(elem.outgoing) == 1 + assert elem.outgoing[0].deptype == 'use' + assert elem.outgoing[0].toElement.name == 'b.py'