Merge pull request #1101 from projectsyn/feat/parallel-alias-setup

Fix performance regression by parallelizing alias checkout

Author: simu

commodore/component/__init__.py

@@ -19,7 +19,7 @@ class Component:
     _version: Optional[str] = None
     _dir: P
     _sub_path: str
-    _aliases: dict[str, tuple[str, str]]
+    _aliases: dict[str, tuple[str, str, MultiDependency]]
     _work_dir: Optional[P]
 
     @classmethod
@@ -58,7 +58,13 @@ def __init__(
         self.version = version
         self._sub_path = sub_path
         self._repo = None
-        self._aliases = {self.name: (self.version or "", self.sub_path or "")}
+        self._aliases = {
+            self.name: (
+                self.version or "",
+                self.sub_path or "",
+                self._dependency,
+            )
+        }
         self._work_dir = work_dir
 
     @property
@@ -137,13 +143,16 @@ def defaults_file(self) -> P:
         return self.alias_defaults_file(self.name)
 
     def alias_directory(self, alias: str) -> P:
-        apath = self._dependency.get_component(alias)
-        if not apath:
-            raise ValueError(f"unknown alias {alias} for component {self.name}")
         if alias not in self._aliases:
             raise ValueError(
                 f"alias {alias} for component {self.name} has not been registered"
             )
+        adep = self._aliases[alias][2]
+        apath = adep.get_component(alias)
+        # Here: if alias is registered in `self._aliases` it must be registered on the
+        # alias's multi-dependency. The assert makes mypy happy. We disable bandit's
+        # "assert_used" lint, since we don't rely on this assertion for correctness.
+        assert apath  # nosec B101
         return apath / self._aliases[alias][1]
 
     def alias_class_file(self, alias: str) -> P:
@@ -190,9 +199,14 @@ def register_alias(
         self,
         alias: str,
         version: str,
+        dependency: MultiDependency,
         sub_path: str = "",
         target_dir: Optional[P] = None,
     ):
+        if alias in self._aliases:
+            raise ValueError(
+                f"alias {alias} already registered on component {self.name}"
+            )
         alias_target_dir = target_dir
         if not alias_target_dir:
             if not self._work_dir:
@@ -201,25 +215,16 @@ def register_alias(
                     + "which isn't configured with a working directory"
                 )
             alias_target_dir = component_dir(self._work_dir, alias)
-        if alias in self._aliases:
-            raise ValueError(
-                f"alias {alias} already registered on component {self.name}"
-            )
-        self._aliases[alias] = (version, sub_path)
-        self._dependency.register_component(alias, alias_target_dir)
+        self._aliases[alias] = (version, sub_path, dependency)
+        dependency.register_component(alias, alias_target_dir)
 
-    def checkout_alias(
-        self, alias: str, alias_dependency: Optional[MultiDependency] = None
-    ):
+    def checkout_alias(self, alias: str):
         if alias not in self._aliases:
             raise ValueError(
                 f"alias {alias} is not registered on component {self.name}"
             )
-
-        if alias_dependency:
-            alias_dependency.checkout_component(alias, self._aliases[alias][0])
-        else:
-            self._dependency.checkout_component(alias, self._aliases[alias][0])
+        adep = self._aliases[alias][2]
+        adep.checkout_component(alias, self._aliases[alias][0])
 
     def is_checked_out(self) -> bool:
         return self.target_dir is not None and self.target_dir.is_dir()

commodore/component/compile.py

@@ -169,7 +169,7 @@ def _setup_component(
     config.register_component_aliases({instance_name: component_name})
     if instance_name != component_name:
         component.register_alias(
-            instance_name, "", sub_path=sub_path, target_dir=target_dir
+            instance_name, "", cdep, sub_path=sub_path, target_dir=target_dir
         )
 
     # Validate component libraries

commodore/dependency_mgmt/__init__.py

@@ -2,6 +2,7 @@
 
 import itertools
 from concurrent.futures import ThreadPoolExecutor
+from typing import Callable, Iterable
 
 import click
 
@@ -109,29 +110,37 @@ def fetch_components(cfg: Config):
                 + "Please specify `--force` to discard them"
             )
         deps.setdefault(cdep.url, []).append(c)
-    fetch_parallel(fetch_component, cfg, deps.values())
+    do_parallel(fetch_component, cfg, deps.values())
 
     components = cfg.get_components()
 
+    aliases: dict[str, list] = {}
     for alias, component in component_aliases.items():
         if alias == component:
             # Nothing to setup for identity alias
             continue
 
         c = components[component]
         aspec = cspecs[alias]
-        adep = None
+        adep = c.dependency
         if aspec.url != c.repo_url:
             adep = cfg.register_dependency_repo(aspec.url)
-            adep.register_component(alias, component_dir(cfg.work_dir, alias))
-
-        c.register_alias(alias, aspec.version, aspec.path)
-        c.checkout_alias(alias, adep)
-
-        create_alias_symlinks(cfg, c, alias)
-
-
-def fetch_component(cfg, dependencies):
+        c.register_alias(alias, aspec.version, adep, aspec.path)
+        if adep.url in deps:
+            # NOTE(sg): if we already processed the dependency URL in the previous fetch
+            # stage, we can create all instance worktrees in parallel. We do so by using
+            # the alias name as the key for our "parallelization" dict.
+            aliases[alias] = [(alias, c)]
+        else:
+            # Otherwise, we use adep.url as the parallelization key to avoid any race
+            # conditions when creating multiple worktrees from a not-yet-cloned
+            # dependency URL.
+            aliases.setdefault(adep.url, []).append((alias, c))
+
+    do_parallel(setup_alias, cfg, aliases.values())
+
+
+def fetch_component(cfg: Config, dependencies: Iterable):
     """
     Fetch all components of a MultiDependency object.
     """
@@ -141,7 +150,13 @@ def fetch_component(cfg, dependencies):
         create_component_symlinks(cfg, c)
 
 
-def fetch_parallel(fetch_fun, cfg, to_fetch):
+def setup_alias(cfg: Config, aliases: Iterable):
+    for alias, c in aliases:
+        c.checkout_alias(alias)
+        create_alias_symlinks(cfg, c, alias)
+
+
+def do_parallel(fun: Callable[[Config, Iterable], None], cfg: Config, data: Iterable):
     """
     Fetch dependencies in parallel threads with ThreadPoolExecutor.
     """
@@ -150,7 +165,7 @@ def fetch_parallel(fetch_fun, cfg, to_fetch):
         # that any exceptions raised in `fetch_fun` are propagated, cf.
         # https://docs.python.org/3/library/concurrent.futures.html#executor-objects. We
         # do so by simply materializing the iterator into a list.
-        list(exe.map(fetch_fun, itertools.repeat(cfg), to_fetch))
+        list(exe.map(fun, itertools.repeat(cfg), data))
 
 
 def register_components(cfg: Config):
@@ -208,10 +223,10 @@ def register_components(cfg: Config):
         c = registered_components[cn]
         aspec = cspecs[alias]
 
+        adep = c.dependency
         if aspec.url != c.repo_url:
             adep = cfg.register_dependency_repo(aspec.url)
-            adep.register_component(alias, component_dir(cfg.work_dir, alias))
-        c.register_alias(alias, aspec.version, aspec.path)
+        c.register_alias(alias, aspec.version, adep, aspec.path)
 
         if not component_dir(cfg.work_dir, alias).is_dir():
             raise click.ClickException(f"Missing alias checkout for '{alias} as {cn}'")
@@ -249,10 +264,10 @@ def fetch_packages(cfg: Config):
                 + "Please specify `--force` to discard them"
             )
         deps.setdefault(pdep.url, []).append((p, pkg))
-    fetch_parallel(fetch_package, cfg, deps.values())
+    do_parallel(fetch_package, cfg, deps.values())
 
 
-def fetch_package(cfg, dependencies):
+def fetch_package(cfg: Config, dependencies: Iterable):
     """
     Fetch all package dependencies of a MultiDependency object.
     """

tests/test_component.py

@@ -485,14 +485,14 @@ def test_component_register_alias_workdir(tmp_path: P, workdir: bool):
 
     if not workdir:
         with pytest.raises(ValueError) as exc:
-            c.register_alias("test-alias", c.version)
+            c.register_alias("test-alias", c.version, c.dependency)
 
         assert (
             "Can't register alias on component test-component which isn't configured with a working directory"
             in str(exc.value)
         )
     else:
-        c.register_alias("test-alias", c.version)
+        c.register_alias("test-alias", c.version, c.dependency)
         assert (
             c.alias_directory("test-alias") == tmp_path / "dependencies" / "test-alias"
         )
@@ -505,6 +505,24 @@ def test_component_register_alias_targetdir(tmp_path: P, workdir: bool):
     if workdir:
         c._work_dir = tmp_path
 
-    c.register_alias("test-alias", c.version, target_dir=tmp_path / "test-alias")
+    c.register_alias(
+        "test-alias", c.version, c.dependency, target_dir=tmp_path / "test-alias"
+    )
     # explicit `target_dir` has precedence over the component's _work_dir
     assert c.alias_directory("test-alias") == tmp_path / "test-alias"
+
+
+def test_component_register_alias_multiple_err(tmp_path: P):
+    c = _setup_component(tmp_path, name="test-component")
+    c.register_alias(
+        "test-alias", c.version, c.dependency, target_dir=tmp_path / "test-alias"
+    )
+
+    with pytest.raises(ValueError) as exc:
+        c.register_alias(
+            "test-alias", c.version, c.dependency, target_dir=tmp_path / "test-alias"
+        )
+
+    assert "alias test-alias already registered on component test-component" in str(
+        exc.value
+    )

tests/test_postprocess.py

@@ -145,7 +145,7 @@ def _setup(tmp_path, f, alias="test-component"):
     cdep = MultiDependency("https://fake.repo.url/", tmp_path / "dependencies")
     component = Component("test-component", dependency=cdep, work_dir=tmp_path)
     if alias != "test-component":
-        component.register_alias(alias, "master")
+        component.register_alias(alias, "master", cdep)
         os.symlink(component.target_directory, component.alias_directory(alias))
     config.register_component(component)
     aliases = {alias: "test-component"}

tests/test_render_inventory.py

@@ -28,7 +28,7 @@ def _setup(tmp_path: Path):
     os.makedirs(tmp_path / "dependencies" / "test")
     cdep = MockMultiDependency(git.Repo.init(tmp_path / "repo.git"))
     c = Component("test", cdep, work_dir=tmp_path)
-    c.register_alias("test-1", "master")
+    c.register_alias("test-1", "master", cdep)
     os.makedirs(c.class_file.parent)
     # Create alias checkout by symlinking component directory
     os.symlink(c.target_directory, c.alias_directory("test-1"))