XML reader: pool repeated attribute strings, and make attribute filters actually filter

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 <a n="x"/> 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 <a n="x"/> 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
@@ -706,16 +738,20 @@ 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:
                             if name not in self.whitelisted_assoc_attributes:
                                 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 self.ignore_all_elem_attributes:
+                                return
                             if name in self.blacklisted_elem_attributes:
                                 return
                             if self.whitelisted_elem_attributes:
@@ -724,7 +760,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 +782,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 +791,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 +805,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
@@ -777,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 <a> children do.
                     for aname, avalue in list(attrs.items()):
                         if len(aname) > 1:
-                            self.currentRelation[aname] = 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':

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 <a n=".." v=".."/> child, and an attribute written inline on the
+enclosing <e> or <r> 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 = """<model version="2.1">
+  <e n="repo">
+    <e n="a.py" t="file" elem_inline="ei">
+      <a n="elem_child" v="ec" />
+      <r r="2" t="use" assoc_inline="ai">
+        <a n="assoc_child" v="ac" />
+      </r>
+    </e>
+    <e n="b.py" t="file" i="2" elem_inline="ei">
+      <a n="elem_child" v="ec" />
+    </e>
+  </e>
+</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'

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 = """<model version="2.1">
+  <e n="repo">
+    <e n="a.py" t="file" language="python">
+      <a n="analyzer_name" v="python_analyzer" />
+      <r r="2" t="function_ref">
+        <a n="call_context" v="module_level" />
+      </r>
+    </e>
+    <e n="b.py" t="file" language="python" i="2">
+      <a n="analyzer_name" v="python_analyzer" />
+      <r r="3" t="function_ref">
+        <a n="call_context" v="module_level" />
+      </r>
+    </e>
+    <e n="c.py" t="file" language="python" i="3" />
+  </e>
+</model>
+"""
+
+
+# Same shape, but the association attributes are written as XML attributes on <r>.
+INLINE_ASSOC_MODEL = """<model version="2.1">
+  <e n="repo">
+    <e n="a.py">
+      <r r="2" t="function_ref" call_context="module_level" />
+    </e>
+    <e n="b.py" i="2">
+      <r r="3" t="function_ref" call_context="module_level" />
+    </e>
+    <e n="c.py" i="3" />
+  </e>
+</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 <e>, not as <a> 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 <a n=...> 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 <r>."""
+    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 = '<model version="2.1"><e n="repo"><e n="f.py"><a n="novalue" /></e></e></model>'
+    graph = SGraph.parse_xml_file_or_stream(io.StringIO(xml))
+    elem = graph.rootNode.children[0].children[0]
+
+    assert elem.attrs['novalue'] is None