C domain: fix performance regression since 3.0.0 (#12162)

Sphinx 3.0.0 onwards have a significant performance regression from
2.4.4 for the C domain. The cProfile output shows that this is due to
_find_named_symbols using a linear search.

Replace Symbol._children with dicts, one keyed by ident and one keyed by
docname. This lets us remove _find_named_symbols and replace these usage
patterns with fast child lookup. Also use the _anonChildren list to
speed up searching through anonymous children.

Note that dict is guaranteed to maintain insertion order since Python
3.7 so we retain iteration. Whenever iteration is required, we use
_childrenByName.values().

Signed-off-by: Donald Hunter <[email protected]>
Co-authored-by: Jakob Lykke Andersen <[email protected]>
Co-authored-by: Adam Turner <[email protected]>

Author: 3 people

CHANGES.rst

@@ -22,6 +22,10 @@ Features added
 Bugs fixed
 ----------
 
+* #12162: Fix a performance regression in the C domain that has
+  been present since version 3.0.0.
+  Patch by Donald Hunter.
+
 Testing
 -------
 

sphinx/domains/c/_symbol.py

@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-from typing import TYPE_CHECKING, Any, Callable
+from typing import TYPE_CHECKING, Any
 
 from sphinx.domains.c._ast import (
     ASTDeclaration,
@@ -11,7 +11,7 @@
 from sphinx.util import logging
 
 if TYPE_CHECKING:
-    from collections.abc import Iterator
+    from collections.abc import Callable, Iterable, Iterator, Sequence
 
     from typing_extensions import Self
 
@@ -32,7 +32,7 @@ def __str__(self) -> str:
 
 
 class SymbolLookupResult:
-    def __init__(self, symbols: Iterator[Symbol], parentSymbol: Symbol,
+    def __init__(self, symbols: Sequence[Symbol], parentSymbol: Symbol,
                  ident: ASTIdentifier) -> None:
         self.symbols = symbols
         self.parentSymbol = parentSymbol
@@ -102,12 +102,14 @@ def __init__(
         self.isRedeclaration = False
         self._assert_invariants()
 
-        # Remember to modify Symbol.remove if modifications to the parent change.
-        self._children: list[Symbol] = []
-        self._anonChildren: list[Symbol] = []
-        # note: _children includes _anonChildren
+        # These properties store the same children for different access patterns.
+        # ``_add_child()`` and ``_remove_child()`` should be used for modifying them.
+        self._children_by_name: dict[str, Symbol] = {}
+        self._children_by_docname: dict[str, dict[str, Symbol]] = {}
+        self._anon_children: set[Symbol] = set()
+
         if self.parent:
-            self.parent._children.append(self)
+            self.parent._add_child(self)
         if self.declaration:
             self.declaration.symbol = self
 
@@ -117,6 +119,27 @@ def __init__(
     def __repr__(self) -> str:
         return f'<Symbol {self.to_string(indent=0)!r}>'
 
+    @property
+    def _children(self) -> Iterable[Symbol]:
+        return self._children_by_name.values()
+
+    def _add_child(self, child: Symbol) -> None:
+        name = child.ident.name
+        if name in self._children_by_name:
+            # Duplicate so don't add - will be reported in _add_symbols()
+            return
+        self._children_by_name[name] = child
+        self._children_by_docname.setdefault(child.docname, {})[name] = child
+        if child.ident.is_anonymous:
+            self._anon_children.add(child)
+
+    def _remove_child(self, child: Symbol) -> None:
+        name = child.ident.name
+        self._children_by_name.pop(name, None)
+        self._children_by_docname.get(child.docname, {}).pop(name, None)
+        if child.ident.is_anonymous:
+            self._anon_children.discard(child)
+
     def _fill_empty(self, declaration: ASTDeclaration, docname: str, line: int) -> None:
         self._assert_invariants()
         assert self.declaration is None
@@ -157,25 +180,28 @@ def _add_function_params(self) -> None:
             Symbol.debug_indent -= 1
 
     def remove(self) -> None:
-        if self.parent is None:
-            return
-        assert self in self.parent._children
-        self.parent._children.remove(self)
-        self.parent = None
+        if self.parent:
+            self.parent._remove_child(self)
+            self.parent = None
 
     def clear_doc(self, docname: str) -> None:
-        for sChild in self._children:
-            sChild.clear_doc(docname)
-            if sChild.declaration and sChild.docname == docname:
-                sChild.declaration = None
-                sChild.docname = None
-                sChild.line = None
-                if sChild.siblingAbove is not None:
-                    sChild.siblingAbove.siblingBelow = sChild.siblingBelow
-                if sChild.siblingBelow is not None:
-                    sChild.siblingBelow.siblingAbove = sChild.siblingAbove
-                sChild.siblingAbove = None
-                sChild.siblingBelow = None
+        if docname not in self._children_by_docname:
+            for child in self._children:
+                child.clear_doc(docname)
+            return
+
+        children: dict[str, Symbol] = self._children_by_docname.pop(docname)
+        for child in children.values():
+            child.declaration = None
+            child.docname = None
+            child.line = None
+            if child.siblingAbove is not None:
+                child.siblingAbove.siblingBelow = child.siblingBelow
+            if child.siblingBelow is not None:
+                child.siblingBelow.siblingAbove = child.siblingAbove
+            child.siblingAbove = None
+            child.siblingBelow = None
+            self._remove_child(child)
 
     def get_all_symbols(self) -> Iterator[Symbol]:
         yield self
@@ -186,14 +212,6 @@ def get_all_symbols(self) -> Iterator[Symbol]:
     def children(self) -> Iterator[Symbol]:
         yield from self._children
 
-    @property
-    def children_recurse_anon(self) -> Iterator[Symbol]:
-        for c in self._children:
-            yield c
-            if not c.ident.is_anon():
-                continue
-            yield from c.children_recurse_anon
-
     def get_lookup_key(self) -> LookupKey:
         # The pickle files for the environment and for each document are distinct.
         # The environment has all the symbols, but the documents has xrefs that
@@ -224,68 +242,6 @@ def get_full_nested_name(self) -> ASTNestedName:
         names = [s.ident for s in symbols]
         return ASTNestedName(names, rooted=False)
 
-    def _find_first_named_symbol(self, ident: ASTIdentifier,
-                                 matchSelf: bool, recurseInAnon: bool) -> Symbol | None:
-        # TODO: further simplification from C++ to C
-        if Symbol.debug_lookup:
-            Symbol.debug_print("_find_first_named_symbol ->")
-        res = self._find_named_symbols(ident, matchSelf, recurseInAnon,
-                                       searchInSiblings=False)
-        try:
-            return next(res)
-        except StopIteration:
-            return None
-
-    def _find_named_symbols(self, ident: ASTIdentifier,
-                            matchSelf: bool, recurseInAnon: bool,
-                            searchInSiblings: bool) -> Iterator[Symbol]:
-        # TODO: further simplification from C++ to C
-        if Symbol.debug_lookup:
-            Symbol.debug_indent += 1
-            Symbol.debug_print("_find_named_symbols:")
-            Symbol.debug_indent += 1
-            Symbol.debug_print("self:")
-            logger.debug(self.to_string(Symbol.debug_indent + 1, addEndNewline=False))
-            Symbol.debug_print("ident:            ", ident)
-            Symbol.debug_print("matchSelf:        ", matchSelf)
-            Symbol.debug_print("recurseInAnon:    ", recurseInAnon)
-            Symbol.debug_print("searchInSiblings: ", searchInSiblings)
-
-        def candidates() -> Iterator[Symbol]:
-            s = self
-            if Symbol.debug_lookup:
-                Symbol.debug_print("searching in self:")
-                logger.debug(s.to_string(Symbol.debug_indent + 1, addEndNewline=False))
-            while True:
-                if matchSelf:
-                    yield s
-                if recurseInAnon:
-                    yield from s.children_recurse_anon
-                else:
-                    yield from s._children
-
-                if s.siblingAbove is None:
-                    break
-                s = s.siblingAbove
-                if Symbol.debug_lookup:
-                    Symbol.debug_print("searching in sibling:")
-                    logger.debug(s.to_string(Symbol.debug_indent + 1, addEndNewline=False))
-
-        for s in candidates():
-            if Symbol.debug_lookup:
-                Symbol.debug_print("candidate:")
-                logger.debug(s.to_string(Symbol.debug_indent + 1, addEndNewline=False))
-            if s.ident == ident:
-                if Symbol.debug_lookup:
-                    Symbol.debug_indent += 1
-                    Symbol.debug_print("matches")
-                    Symbol.debug_indent -= 3
-                yield s
-                if Symbol.debug_lookup:
-                    Symbol.debug_indent += 2
-        if Symbol.debug_lookup:
-            Symbol.debug_indent -= 2
-
     def _symbol_lookup(
         self,
         nestedName: ASTNestedName,
@@ -314,16 +270,14 @@ def _symbol_lookup(
         # find the right starting point for lookup
         parentSymbol = self
         if nestedName.rooted:
-            while parentSymbol.parent:
+            while parentSymbol.parent is not None:
                 parentSymbol = parentSymbol.parent
+
         if ancestorLookupType is not None:
             # walk up until we find the first identifier
             firstName = names[0]
             while parentSymbol.parent:
-                if parentSymbol.find_identifier(firstName,
-                                                matchSelf=matchSelf,
-                                                recurseInAnon=recurseInAnon,
-                                                searchInSiblings=searchInSiblings):
+                if firstName.name in parentSymbol._children_by_name:
                     break
                 parentSymbol = parentSymbol.parent
 
@@ -333,18 +287,15 @@ def _symbol_lookup(
 
         # and now the actual lookup
         for ident in names[:-1]:
-            symbol = parentSymbol._find_first_named_symbol(
-                ident, matchSelf=matchSelf, recurseInAnon=recurseInAnon)
-            if symbol is None:
+            name = ident.name
+            if name in parentSymbol._children_by_name:
+                symbol = parentSymbol._children_by_name[name]
+            else:
                 symbol = onMissingQualifiedSymbol(parentSymbol, ident)
                 if symbol is None:
                     if Symbol.debug_lookup:
                         Symbol.debug_indent -= 2
                     return None
-            # We have now matched part of a nested name, and need to match more
-            # so even if we should matchSelf before, we definitely shouldn't
-            # even more. (see also issue #2666)
-            matchSelf = False
             parentSymbol = symbol
 
         if Symbol.debug_lookup:
@@ -353,15 +304,19 @@ def _symbol_lookup(
 
         # handle the last name
         ident = names[-1]
+        name = ident.name
+        symbol = parentSymbol._children_by_name.get(name)
+        if not symbol and recurseInAnon:
+            for child in parentSymbol._anon_children:
+                if name in child._children_by_name:
+                    symbol = child._children_by_name[name]
+                    break
 
-        symbols = parentSymbol._find_named_symbols(
-            ident, matchSelf=matchSelf,
-            recurseInAnon=recurseInAnon,
-            searchInSiblings=searchInSiblings)
         if Symbol.debug_lookup:
-            symbols = list(symbols)  # type: ignore[assignment]
             Symbol.debug_indent -= 2
-        return SymbolLookupResult(symbols, parentSymbol, ident)
+
+        result = [symbol] if symbol else []
+        return SymbolLookupResult(result, parentSymbol, ident)
 
     def _add_symbols(
         self,
@@ -535,17 +490,17 @@ def merge_with(self, other: Symbol, docnames: list[str],
         if Symbol.debug_lookup:
             Symbol.debug_indent += 1
             Symbol.debug_print("merge_with:")
+
         assert other is not None
         for otherChild in other._children:
-            ourChild = self._find_first_named_symbol(
-                ident=otherChild.ident, matchSelf=False,
-                recurseInAnon=False)
-            if ourChild is None:
+            otherName = otherChild.ident.name
+            if otherName not in self._children_by_name:
                 # TODO: hmm, should we prune by docnames?
-                self._children.append(otherChild)
                 otherChild.parent = self
+                self._add_child(otherChild)
                 otherChild._assert_invariants()
                 continue
+            ourChild = self._children_by_name[otherName]
             if otherChild.declaration and otherChild.docname in docnames:
                 if not ourChild.declaration:
                     ourChild._fill_empty(otherChild.declaration,
@@ -563,6 +518,7 @@ def merge_with(self, other: Symbol, docnames: list[str],
                     # just ignore it, right?
                     pass
             ourChild.merge_with(otherChild, docnames, env)
+
         if Symbol.debug_lookup:
             Symbol.debug_indent -= 1
 
@@ -611,10 +567,13 @@ def find_identifier(self, ident: ASTIdentifier,
                 Symbol.debug_indent -= 2
             if matchSelf and current.ident == ident:
                 return current
-            children = current.children_recurse_anon if recurseInAnon else current._children
-            for s in children:
-                if s.ident == ident:
-                    return s
+            name = ident.name
+            if name in current._children_by_name:
+                return current._children_by_name[name]
+            if recurseInAnon:
+                for child in current._anon_children:
+                    if name in child._children_by_name:
+                        return child._children_by_name[name]
             if not searchInSiblings:
                 break
             current = current.siblingAbove
@@ -626,24 +585,17 @@ def direct_lookup(self, key: LookupKey) -> Symbol | None:
             Symbol.debug_print("direct_lookup:")
             Symbol.debug_indent += 1
         s = self
-        for name, id_ in key.data:
-            res = None
-            for cand in s._children:
-                if cand.ident == name:
-                    res = cand
-                    break
-            s = res
+        for ident, id_ in key.data:
+            s = s._children_by_name.get(ident.name)
             if Symbol.debug_lookup:
-                Symbol.debug_print("name:          ", name)
+                Symbol.debug_print("name:          ", ident.name)
                 Symbol.debug_print("id:            ", id_)
                 if s is not None:
                     logger.debug(s.to_string(Symbol.debug_indent + 1, addEndNewline=False))
                 else:
                     Symbol.debug_print("not found")
             if s is None:
-                if Symbol.debug_lookup:
-                    Symbol.debug_indent -= 2
-                return None
+                break
         if Symbol.debug_lookup:
             Symbol.debug_indent -= 2
         return s
@@ -683,7 +635,7 @@ def to_string(self, indent: int, *, addEndNewline: bool = True) -> str:
             res.append('::')
         else:
             if self.ident:
-                res.append(str(self.ident))
+                res.append(self.ident.name)
             else:
                 res.append(str(self.declaration))
             if self.declaration: