Parse only if necessary (#212)

Parse only if necessary

Parsing before every macro expansion etc. is unnecessary and can cause performance issues, because parsing is a really slow operation.
Parse only after a context switch (necessary because all Specfile instances share a single global macro context) or if something that affects parsing has actually changed.
Related to rebase-helper/rebase-helper#910.
RELEASE NOTES BEGIN
Parsing the spec file by RPM is now performed only if really necessary, greatly improving performance in certain scenarios.
RELEASE NOTES END

Reviewed-by: Jiri Popelka

Author: softwarefactory-project-zuul[bot]

specfile/context_management.py

@@ -39,7 +39,7 @@ def capture_stderr() -> Generator[List[bytes], None, None]:
 
 class GeneratorContextManager(contextlib._GeneratorContextManager):
     """
-    Extended contextlib._GeneratorContextManager that provides get() method.
+    Extended contextlib._GeneratorContextManager that provides content property.
     """
 
     def __init__(self, function: Callable) -> None:

specfile/sections.py

@@ -231,12 +231,7 @@ def parse(
 
         def expand(s):
             if context:
-                result = context.expand(
-                    s, skip_parsing=getattr(expand, "skip_parsing", False)
-                )
-                # parse only once
-                expand.skip_parsing = True
-                return result
+                return context.expand(s)
             return Macros.expand(s)
 
         def split_id(line):

specfile/sources.py

@@ -5,14 +5,17 @@
 import re
 import urllib.parse
 from abc import ABC, abstractmethod
-from typing import Iterable, List, Optional, Tuple, Union, cast, overload
+from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple, Union, cast, overload
 
 from specfile.exceptions import DuplicateSourceException
 from specfile.formatter import formatted
 from specfile.sourcelist import Sourcelist, SourcelistEntry
 from specfile.tags import Comments, Tag, Tags
 from specfile.utils import get_filename_from_location
 
+if TYPE_CHECKING:
+    from specfile.specfile import Specfile
+
 
 class Source(ABC):
     """Class that represents a source."""
@@ -236,6 +239,7 @@ def __init__(
         allow_duplicates: bool = False,
         default_to_implicit_numbering: bool = False,
         default_source_number_digits: int = 1,
+        context: Optional["Specfile"] = None,
     ) -> None:
         """
         Constructs a `Sources` object.
@@ -256,6 +260,7 @@ def __init__(
         self._allow_duplicates = allow_duplicates
         self._default_to_implicit_numbering = default_to_implicit_numbering
         self._default_source_number_digits = default_source_number_digits
+        self._context = context
 
     def __eq__(self, other: object) -> bool:
         if not isinstance(other, Sources):
@@ -277,7 +282,7 @@ def __repr__(self) -> str:
         return (
             f"{self.__class__.__name__}({self._tags!r}, {self._sourcelists!r}, "
             f"{self._allow_duplicates!r}, {self._default_to_implicit_numbering!r}, "
-            f"{self._default_source_number_digits!r})"
+            f"{self._default_source_number_digits!r}, {self._context!r})"
         )
 
     def __contains__(self, location: object) -> bool:
@@ -502,21 +507,25 @@ def insert(self, i: int, location: str) -> None:
                 name, separator = self._get_tag_format(cast(TagSource, source), number)
                 container.insert(
                     index,
-                    Tag(name, location, separator, Comments()),
+                    Tag(name, location, separator, Comments(), context=self._context),
                 )
                 self._deduplicate_tag_names(i)
             else:
                 container.insert(
                     index,
-                    SourcelistEntry(location, Comments()),  # type: ignore[arg-type]
+                    SourcelistEntry(  # type: ignore[arg-type]
+                        location, Comments(), context=self._context
+                    ),
                 )
         elif self._sourcelists:
-            self._sourcelists[-1].append(SourcelistEntry(location, Comments()))
+            self._sourcelists[-1].append(
+                SourcelistEntry(location, Comments(), context=self._context)
+            )
         else:
             index, name, separator = self._get_initial_tag_setup()
             self._tags.insert(
                 index,
-                Tag(name, location, separator, Comments()),
+                Tag(name, location, separator, Comments(), context=self._context),
             )
 
     def insert_numbered(self, number: int, location: str) -> int:
@@ -549,7 +558,9 @@ def insert_numbered(self, number: int, location: str) -> int:
         else:
             i = 0
             index, name, separator = self._get_initial_tag_setup(number)
-        self._tags.insert(index, Tag(name, location, separator, Comments()))
+        self._tags.insert(
+            index, Tag(name, location, separator, Comments(), context=self._context)
+        )
         self._deduplicate_tag_names(i)
         return i
 

specfile/spec_parser.py

@@ -3,8 +3,10 @@
 
 import contextlib
 import copy
+import hashlib
 import logging
 import os
+import pickle
 import re
 import tempfile
 from pathlib import Path
@@ -41,6 +43,9 @@ class SpecParser:
           and were replaced with dummy files.
     """
 
+    # hash of input parameters to the last parse performed
+    _last_parse_hash = None
+
     def __init__(
         self,
         sourcedir: Path,
@@ -325,11 +330,32 @@ def parse(
         Raises:
             RPMException, if parsing error occurs.
         """
+        # calculate hash of all input parameters
+        payload = (
+            id(self),
+            self.sourcedir,
+            self.macros,
+            self.force_parse,
+            content,
+            extra_macros,
+        )
+        parse_hash = hashlib.sha256(
+            pickle.dumps(payload, protocol=pickle.HIGHEST_PROTOCOL)
+        ).digest()
+        if parse_hash == SpecParser._last_parse_hash:
+            # none of the input parameters has changed, no need to parse again
+            return
         if self.spec:
             # workaround RPM lua tables feature/bug, see above for details
             del self.spec
         try:
-            self.spec, self.tainted = self._do_parse(content, extra_macros)
+            try:
+                self.spec, self.tainted = self._do_parse(content, extra_macros)
+            except Exception:
+                SpecParser._last_parse_hash = None
+                raise
+            else:
+                SpecParser._last_parse_hash = parse_hash
         except RPMException:
             self.spec = None
             self.tainted = False

specfile/specfile.py

@@ -64,7 +64,6 @@ def __init__(
         self._parser = SpecParser(
             Path(sourcedir or self.path.parent), macros, force_parse
         )
-        # parse here to fail early on parsing errors
         self._parser.parse(str(self))
 
     def __eq__(self, other: object) -> bool:
@@ -161,23 +160,18 @@ def expand(
         self,
         expression: str,
         extra_macros: Optional[List[Tuple[str, str]]] = None,
-        skip_parsing: bool = False,
     ) -> str:
         """
         Expands an expression in the context of the spec file.
 
         Args:
             expression: Expression to expand.
             extra_macros: Extra macros to be defined before expansion is performed.
-            skip_parsing: Do not parse the spec file before expansion is performed.
-              Defaults to False. Mutually exclusive with extra_macros. Set this to True
-              only if you are certain that the global macro context is up-to-date.
 
         Returns:
             Expanded expression.
         """
-        if not skip_parsing:
-            self._parser.parse(str(self), extra_macros)
+        self._parser.parse(str(self), extra_macros)
         return Macros.expand(expression)
 
     def get_active_macros(self) -> List[Macro]:
@@ -336,6 +330,7 @@ def sources(
                     allow_duplicates,
                     default_to_implicit_numbering,
                     default_source_number_digits,
+                    context=self,
                 )
             finally:
                 for section, sourcelist in sourcelists:
@@ -372,6 +367,7 @@ def patches(
                     allow_duplicates,
                     default_to_implicit_numbering,
                     default_source_number_digits,
+                    context=self,
                 )
             finally:
                 for section, patchlist in patchlists:

specfile/value_parser.py

@@ -312,12 +312,7 @@ def construct_regex(
 
         def expand(s):
             if context:
-                result = context.expand(
-                    s, skip_parsing=getattr(expand, "skip_parsing", False)
-                )
-                # parse only once
-                expand.skip_parsing = True
-                return result
+                return context.expand(s)
             return Macros.expand(s)
 
         def flatten(nodes):

tests/integration/test_specfile.py

@@ -12,7 +12,7 @@
 from specfile.exceptions import RPMException, SpecfileException
 from specfile.prep import AutopatchMacro, AutosetupMacro, PatchMacro, SetupMacro
 from specfile.sections import Section
-from specfile.specfile import Specfile
+from specfile.specfile import Specfile, SpecParser
 
 
 def test_parse(spec_multiple_sources):
@@ -481,3 +481,21 @@ def test_copy(spec_autosetup):
     assert deep_copy is not spec
     assert deep_copy._lines is not spec._lines
     assert deep_copy._parser is not spec._parser
+
+
+def test_parse_if_necessary(spec_macros):
+    flexmock(SpecParser).should_call("_do_parse").once()
+    spec1 = Specfile(spec_macros)
+    spec2 = copy.deepcopy(spec1)
+    flexmock(SpecParser).should_call("_do_parse").never()
+    assert spec1.expanded_name == "test"
+    flexmock(SpecParser).should_call("_do_parse").once()
+    assert spec2.expanded_name == "test"
+    assert spec2.expanded_version == "0.1.2~rc2"
+    flexmock(SpecParser).should_call("_do_parse").once()
+    assert spec1.expanded_version == "0.1.2~rc2"
+    with spec1.macro_definitions() as md:
+        md[0].body = "28"
+    flexmock(SpecParser).should_call("_do_parse").once()
+    assert spec1.expanded_name == "test"
+    assert spec1.expanded_version == "28.1.2~rc2"