Merge pull request #11868 from DefaultRyan/normalize-path-cached

cache normalize_path in req_uninstall and is_local

Author: sbidoul

news/11889.bugfix.rst

@@ -0,0 +1,4 @@
+The ``uninstall`` and ``install --force-reinstall`` commands no longer call
+``normalize_path()`` repeatedly on the same paths. Instead, these results are
+cached for the duration of an uninstall operation, resulting in improved
+performance, particularly on Windows.

src/pip/_internal/req/req_uninstall.py

@@ -11,8 +11,9 @@
 from pip._internal.utils.compat import WINDOWS
 from pip._internal.utils.egg_link import egg_link_path_from_location
 from pip._internal.utils.logging import getLogger, indent_log
-from pip._internal.utils.misc import ask, is_local, normalize_path, renames, rmtree
+from pip._internal.utils.misc import ask, normalize_path, renames, rmtree
 from pip._internal.utils.temp_dir import AdjacentTempDirectory, TempDirectory
+from pip._internal.utils.virtualenv import running_under_virtualenv
 
 logger = getLogger(__name__)
 
@@ -312,21 +313,28 @@ def __init__(self, dist: BaseDistribution) -> None:
         self._pth: Dict[str, UninstallPthEntries] = {}
         self._dist = dist
         self._moved_paths = StashedUninstallPathSet()
+        # Create local cache of normalize_path results. Creating an UninstallPathSet
+        # can result in hundreds/thousands of redundant calls to normalize_path with
+        # the same args, which hurts performance.
+        self._normalize_path_cached = functools.lru_cache()(normalize_path)
 
     def _permitted(self, path: str) -> bool:
         """
         Return True if the given path is one we are permitted to
         remove/modify, False otherwise.
 
         """
-        return is_local(path)
+        # aka is_local, but caching normalized sys.prefix
+        if not running_under_virtualenv():
+            return True
+        return path.startswith(self._normalize_path_cached(sys.prefix))
 
     def add(self, path: str) -> None:
         head, tail = os.path.split(path)
 
         # we normalize the head to resolve parent directory symlinks, but not
         # the tail, since we only want to uninstall symlinks, not their targets
-        path = os.path.join(normalize_path(head), os.path.normcase(tail))
+        path = os.path.join(self._normalize_path_cached(head), os.path.normcase(tail))
 
         if not os.path.exists(path):
             return
@@ -341,7 +349,7 @@ def add(self, path: str) -> None:
             self.add(cache_from_source(path))
 
     def add_pth(self, pth_file: str, entry: str) -> None:
-        pth_file = normalize_path(pth_file)
+        pth_file = self._normalize_path_cached(pth_file)
         if self._permitted(pth_file):
             if pth_file not in self._pth:
                 self._pth[pth_file] = UninstallPthEntries(pth_file)
@@ -531,7 +539,9 @@ def from_dist(cls, dist: BaseDistribution) -> "UninstallPathSet":
             # above, so this only covers the setuptools-style editable.
             with open(develop_egg_link) as fh:
                 link_pointer = os.path.normcase(fh.readline().strip())
-                normalized_link_pointer = normalize_path(link_pointer)
+                normalized_link_pointer = paths_to_remove._normalize_path_cached(
+                    link_pointer
+                )
             assert os.path.samefile(
                 normalized_link_pointer, normalized_dist_location
             ), (

tests/unit/test_req_uninstall.py

@@ -21,7 +21,7 @@
 
 # Pretend all files are local, so UninstallPathSet accepts files in the tmpdir,
 # outside the virtualenv
-def mock_is_local(path: str) -> bool:
+def mock_permitted(ups: UninstallPathSet, path: str) -> bool:
     return True
 
 
@@ -129,7 +129,11 @@ def in_tmpdir(paths: List[str]) -> List[str]:
 
 class TestUninstallPathSet:
     def test_add(self, tmpdir: Path, monkeypatch: pytest.MonkeyPatch) -> None:
-        monkeypatch.setattr(pip._internal.req.req_uninstall, "is_local", mock_is_local)
+        monkeypatch.setattr(
+            pip._internal.req.req_uninstall.UninstallPathSet,
+            "_permitted",
+            mock_permitted,
+        )
         # Fix case for windows tests
         file_extant = os.path.normcase(os.path.join(tmpdir, "foo"))
         file_nonexistent = os.path.normcase(os.path.join(tmpdir, "nonexistent"))
@@ -145,7 +149,11 @@ def test_add(self, tmpdir: Path, monkeypatch: pytest.MonkeyPatch) -> None:
         assert ups._paths == {file_extant}
 
     def test_add_pth(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
-        monkeypatch.setattr(pip._internal.req.req_uninstall, "is_local", mock_is_local)
+        monkeypatch.setattr(
+            pip._internal.req.req_uninstall.UninstallPathSet,
+            "_permitted",
+            mock_permitted,
+        )
         # Fix case for windows tests
         tmpdir = os.path.normcase(tmp_path)
         on_windows = sys.platform == "win32"
@@ -175,7 +183,11 @@ def test_add_pth(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
 
     @pytest.mark.skipif("sys.platform == 'win32'")
     def test_add_symlink(self, tmpdir: Path, monkeypatch: pytest.MonkeyPatch) -> None:
-        monkeypatch.setattr(pip._internal.req.req_uninstall, "is_local", mock_is_local)
+        monkeypatch.setattr(
+            pip._internal.req.req_uninstall.UninstallPathSet,
+            "_permitted",
+            mock_permitted,
+        )
         f = os.path.join(tmpdir, "foo")
         with open(f, "w"):
             pass
@@ -187,7 +199,11 @@ def test_add_symlink(self, tmpdir: Path, monkeypatch: pytest.MonkeyPatch) -> Non
         assert ups._paths == {foo_link}
 
     def test_compact_shorter_path(self, monkeypatch: pytest.MonkeyPatch) -> None:
-        monkeypatch.setattr(pip._internal.req.req_uninstall, "is_local", mock_is_local)
+        monkeypatch.setattr(
+            pip._internal.req.req_uninstall.UninstallPathSet,
+            "_permitted",
+            mock_permitted,
+        )
         monkeypatch.setattr("os.path.exists", lambda p: True)
         # This deals with nt/posix path differences
         short_path = os.path.normcase(
@@ -202,7 +218,11 @@ def test_compact_shorter_path(self, monkeypatch: pytest.MonkeyPatch) -> None:
     def test_detect_symlink_dirs(
         self, monkeypatch: pytest.MonkeyPatch, tmpdir: Path
     ) -> None:
-        monkeypatch.setattr(pip._internal.req.req_uninstall, "is_local", mock_is_local)
+        monkeypatch.setattr(
+            pip._internal.req.req_uninstall.UninstallPathSet,
+            "_permitted",
+            mock_permitted,
+        )
 
         # construct 2 paths:
         #  tmpdir/dir/file