From 5361145dfbe555b0b0d049cbe8fa69981bea4abb Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 23 Oct 2026 10:21:04 +0200 Subject: [PATCH] perf: consolidate reported import and lookup improvements Address the four distinct findings in #3264, #3269, #3287, #3306, #3340 and #3365. Add deterministic operation-count and behavioral regressions without claiming global startup or end-to-end complexity improvements. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8263c5a8-35c8-4a1d-97f5-ea68d8d4fddc --- .../docs/contributing/development-guide.md | 9 + .../commands/marketplace/plugin/__init__.py | 3 +- src/apm_cli/commands/uninstall/engine.py | 6 +- src/apm_cli/commands/update.py | 3 +- src/apm_cli/deps/download_strategies.py | 15 +- .../unit/test_performance_scan_regressions.py | 261 ++++++++++++++++++ 6 files changed, 287 insertions(+), 10 deletions(-) create mode 100644 tests/unit/test_performance_scan_regressions.py diff --git a/docs/src/content/docs/contributing/development-guide.md b/docs/src/content/docs/contributing/development-guide.md index fbfdee9e5e..2b3a13517c 100644 --- a/docs/src/content/docs/contributing/development-guide.md +++ b/docs/src/content/docs/contributing/development-guide.md @@ -201,6 +201,15 @@ uv run pytest tests/unit -x -v to force serial execution. The default selection in `pyproject.toml` excludes `benchmark` and `live` tests. +For performance changes, prefer deterministic operation-count regressions over +wall-clock thresholds. Run the consolidated scan regressions with +`uv run --extra dev pytest -q tests/unit/test_performance_scan_regressions.py`. +They check deferred error-only imports, at most two indexed ZIP member lookups, +and uninstall deduplication without comparing previously selected entries. +Archive parsing and canonical dependency matching retain their existing costs. +Deferring a direct import does not prove a cold-start speedup: other modules may +still load the same dependency transitively. + Without `uv`, use a standard Python venv and pip: ```bash diff --git a/src/apm_cli/commands/marketplace/plugin/__init__.py b/src/apm_cli/commands/marketplace/plugin/__init__.py index 7f9d884c5a..1a82746ee7 100644 --- a/src/apm_cli/commands/marketplace/plugin/__init__.py +++ b/src/apm_cli/commands/marketplace/plugin/__init__.py @@ -7,7 +7,6 @@ from pathlib import Path import click -import yaml from ....core.command_logger import CommandLogger from ....marketplace.errors import ( @@ -60,6 +59,8 @@ def _has_marketplace_block(apm_path: Path) -> bool: """Return True when *apm_path* has a populated ``marketplace:`` block.""" if not apm_path.exists(): return False + import yaml + try: # Bounded loader so a hostile apm.yml cannot wedge this probe with a # merge/alias expansion bomb (fails closed as yaml.YAMLError). diff --git a/src/apm_cli/commands/uninstall/engine.py b/src/apm_cli/commands/uninstall/engine.py index 3541ac09a0..0176bfae31 100644 --- a/src/apm_cli/commands/uninstall/engine.py +++ b/src/apm_cli/commands/uninstall/engine.py @@ -663,6 +663,7 @@ def _validate_uninstall_packages( ) packages_to_remove = [] + seen_entry_ids: set[int] = set() packages_not_found = [] for package in packages: @@ -702,7 +703,10 @@ def _validate_uninstall_packages( selection = select_manifest_dependency(canonical_for_match, current_deps, lockfile) if selection.status is DependencySelectionStatus.MATCHED: matched_dep = selection.manifest_entry - if matched_dep not in packages_to_remove: + # Selection returns the original manifest object, including unhashable mappings. + entry_id = id(matched_dep) + if entry_id not in seen_entry_ids: + seen_entry_ids.add(entry_id) packages_to_remove.append(matched_dep) if not log_matches: continue diff --git a/src/apm_cli/commands/update.py b/src/apm_cli/commands/update.py index 6c01be4dad..8466692e53 100644 --- a/src/apm_cli/commands/update.py +++ b/src/apm_cli/commands/update.py @@ -59,7 +59,6 @@ from typing import TYPE_CHECKING, Any import click -from git.exc import GitCommandError from ..core.auth import AuthResolver from ..core.command_logger import InstallLogger @@ -156,6 +155,8 @@ def _resolve_and_stage_revision_pin_updates( against the new SHAs after the user's consent decision while dry-run and decline paths leave the original manifest model untouched. """ + from git.exc import GitCommandError + only_set = set(only_packages) if only_packages is not None else None logger.progress("Checking upstream for revision-pin freshness...", symbol="running") diff --git a/src/apm_cli/deps/download_strategies.py b/src/apm_cli/deps/download_strategies.py index d0424ce533..bdbd22d5d7 100644 --- a/src/apm_cli/deps/download_strategies.py +++ b/src/apm_cli/deps/download_strategies.py @@ -549,13 +549,14 @@ def download_file_from_artifactory( if resp.status_code != 200: continue with zipfile.ZipFile(io.BytesIO(resp.content)) as zf: - names = zf.namelist() - root_prefix = names[0] if names else "" - target_name = root_prefix + file_path - if target_name in names: - return zf.read(target_name) - if file_path in names: - return zf.read(file_path) + entries = zf.infolist() + root_prefix = entries[0].filename if entries else "" + for name in (root_prefix + file_path, file_path): + try: + entry = zf.getinfo(name) + except KeyError: + continue + return zf.read(entry) except (zipfile.BadZipFile, requests.RequestException): continue diff --git a/tests/unit/test_performance_scan_regressions.py b/tests/unit/test_performance_scan_regressions.py new file mode 100644 index 0000000000..9acccfdd4b --- /dev/null +++ b/tests/unit/test_performance_scan_regressions.py @@ -0,0 +1,261 @@ +"""Deterministic regressions for the consolidated October performance scans.""" + +from __future__ import annotations + +import io +import json +import subprocess +import sys +import zipfile +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + +from apm_cli.commands.marketplace.plugin import _has_marketplace_block +from apm_cli.commands.uninstall.engine import _validate_uninstall_packages +from apm_cli.deps.download_strategies import DownloadDelegate +from apm_cli.deps.lockfile import LockedDependency, LockFile +from apm_cli.models.dependency.reference import DependencyReference +from apm_cli.models.dependency.selection import DependencySelection, DependencySelectionStatus + +pytestmark = pytest.mark.component + + +@pytest.mark.parametrize( + ("module", "dependency"), + [ + ("apm_cli.commands.update", "git.exc"), + ("apm_cli.commands.marketplace.plugin", "yaml"), + ], +) +def test_command_import_defers_its_error_only_dependency(module: str, dependency: str) -> None: + """Check the reported import edge, not unrelated transitive imports.""" + code = """ +import builtins +import importlib +import json +import sys + +module, dependency = sys.argv[1:] +original_import = builtins.__import__ +attempts = [] +def track_import(name, globals=None, locals=None, fromlist=(), level=0): + if name == dependency and globals and globals.get("__name__") == module: + attempts.append(name) + return original_import(name, globals, locals, fromlist, level) +builtins.__import__ = track_import +importlib.import_module(module) +print(json.dumps(attempts)) +""" + result = subprocess.run( + [sys.executable, "-c", code, module, dependency], + capture_output=True, + text=True, + check=False, + timeout=30, + ) + assert result.returncode == 0, result.stderr + assert json.loads(result.stdout) == [] + + +@pytest.mark.parametrize( + ("content", "expected"), + [ + ("marketplace:\n name: example\n", True), + ("marketplace: {}\n", True), + ("marketplace: null\n", False), + ("name: example\n", False), + ("", False), + ("- marketplace\n", False), + ("marketplace: [\n", False), + ], +) +def test_marketplace_probe_retains_yaml_behavior( + tmp_path: Path, content: str, expected: bool +) -> None: + manifest = tmp_path / "apm.yml" + manifest.write_text(content, encoding="utf-8") + assert _has_marketplace_block(manifest) is expected + + +def test_marketplace_probe_retains_missing_and_unreadable_behavior(tmp_path: Path) -> None: + manifest = tmp_path / "apm.yml" + assert _has_marketplace_block(manifest) is False + manifest.write_text("marketplace: {}\n", encoding="utf-8") + with patch("apm_cli.utils.yaml_io.load_yaml", side_effect=OSError("permission denied")): + assert _has_marketplace_block(manifest) is False + + +def _archive(entries: dict[str, bytes]) -> bytes: + """Build real ZIP bytes without touching disk or a remote registry.""" + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w") as archive: + for name, content in entries.items(): + archive.writestr(name, content) + return buffer.getvalue() + + +@pytest.mark.parametrize("unrelated_count", [0, 1000]) +@pytest.mark.parametrize( + ("entries", "expected", "lookups"), + [ + ({"repo/": b"", "repo/apm.yml": b"prefixed", "apm.yml": b"bare"}, b"prefixed", 1), + ({"repo/": b"", "apm.yml": b"bare"}, b"bare", 2), + ({"apm.yml": b"bare"}, b"bare", 2), + ({}, None, 2), + ], +) +def test_archive_member_lookup_is_bounded_and_preserves_precedence( + entries: dict[str, bytes], expected: bytes | None, lookups: int, unrelated_count: int +) -> None: + """No namelist allocation or linear scan, regardless of archive size.""" + archive_bytes = _archive( + {**entries, **{f"other/{index}.txt": b"filler" for index in range(unrelated_count)}} + ) + host = SimpleNamespace( + registry_config=None, + artifactory_token=None, + _resilient_get=MagicMock( + return_value=SimpleNamespace(status_code=200, content=archive_bytes) + ), + ) + original_getinfo = zipfile.ZipFile.getinfo + queried_names: list[str] = [] + + def getinfo(archive: zipfile.ZipFile, name: str) -> zipfile.ZipInfo: + queried_names.append(name) + return original_getinfo(archive, name) + + with ( + patch("apm_cli.deps.artifactory_entry.fetch_entry_from_archive", return_value=None), + patch( + "apm_cli.deps.download_strategies.build_artifactory_archive_url", + return_value=["https://registry.example/archive.zip"], + ), + patch.object(zipfile.ZipFile, "namelist", side_effect=AssertionError("linear scan")), + patch.object(zipfile.ZipFile, "getinfo", getinfo), + ): + delegate = DownloadDelegate(host) + if expected is None: + with pytest.raises(RuntimeError, match="Failed to download file"): + delegate.download_file_from_artifactory( + "registry.example", "apm", "owner", "repo", "apm.yml", "main" + ) + else: + assert ( + delegate.download_file_from_artifactory( + "registry.example", "apm", "owner", "repo", "apm.yml", "main" + ) + == expected + ) + assert len(queried_names) == lookups + assert queried_names[-1] == ("repo/apm.yml" if lookups == 1 else "apm.yml") + + +@pytest.mark.parametrize( + "first_content", + [b"not a zip", _archive({}), _archive({"other.yml": b"other"})], + ids=["invalid-zip", "empty-zip", "missing-member"], +) +def test_archive_missing_member_continues_to_next_candidate(first_content: bytes) -> None: + host = SimpleNamespace( + registry_config=None, + artifactory_token=None, + _resilient_get=MagicMock( + side_effect=[ + SimpleNamespace(status_code=200, content=first_content), + SimpleNamespace(status_code=200, content=_archive({"apm.yml": b"found"})), + ] + ), + ) + with ( + patch("apm_cli.deps.artifactory_entry.fetch_entry_from_archive", return_value=None), + patch( + "apm_cli.deps.download_strategies.build_artifactory_archive_url", + return_value=[ + "https://registry.example/first.zip", + "https://registry.example/second.zip", + ], + ), + ): + result = DownloadDelegate(host).download_file_from_artifactory( + "registry.example", "apm", "owner", "repo", "apm.yml", "main" + ) + assert result == b"found" + assert host._resilient_get.call_count == 2 + + +@pytest.mark.parametrize("entry_kind", ["string", "mapping", "model"]) +@pytest.mark.parametrize("log_matches", [False, True]) +def test_uninstall_deduplication_retains_manifest_objects_and_argument_order( + entry_kind: str, log_matches: bool +) -> None: + declarations = ["org/first", "org/second"] + if entry_kind == "mapping": + entries = [{"git": name, "ref": "main"} for name in declarations] + elif entry_kind == "model": + entries = [DependencyReference.parse(name) for name in declarations] + else: + entries = declarations + logger = MagicMock() + removed, missing = _validate_uninstall_packages( + ["org/second", "org/first", "org/second", "org/missing"], + entries, + logger, + log_matches=log_matches, + ) + assert len(removed) == 2 + assert removed[0] is entries[1] + assert removed[1] is entries[0] + assert missing == ["org/missing"] + assert logger.progress.call_count == (3 if log_matches else 0) + + +def test_uninstall_alias_and_exact_path_remove_one_unhashable_entry() -> None: + entry = {"path": "../packages/example"} + locked = LockedDependency( + repo_url="_local/example", source="local", local_path="../packages/example" + ) + lockfile = LockFile(dependencies={locked.get_unique_key(): locked}) + removed, missing = _validate_uninstall_packages( + ["_local/example", "../packages/example", "_local/example"], + [entry], + MagicMock(), + lockfile=lockfile, + ) + assert len(removed) == 1 + assert removed[0] is entry + assert missing == [] + + +@pytest.mark.parametrize("count", [10, 1000]) +def test_uninstall_deduplication_does_not_compare_previously_selected_entries(count: int) -> None: + """Isolate deduplication cost from the unchanged canonical selection cost.""" + comparisons = 0 + + class CountedEntry(dict): + def __eq__(self, other: object) -> bool: + nonlocal comparisons + comparisons += 1 + return super().__eq__(other) + + entries = [CountedEntry(git=f"org/package-{index}") for index in range(count)] + selections = [ + DependencySelection(DependencySelectionStatus.MATCHED, entry, 1) for entry in entries + ] + with patch( + "apm_cli.commands.uninstall.engine.select_manifest_dependency", + side_effect=selections + selections, + ): + removed, missing = _validate_uninstall_packages( + [entry["git"] for entry in entries] * 2, + entries, + MagicMock(), + log_matches=False, + ) + assert len(removed) == count + assert all(actual is expected for actual, expected in zip(removed, entries, strict=True)) + assert missing == [] + assert comparisons == 0