GH-125413: pathlib: use `scandir()` to speed up `copy()` #126263

barneygale · 2024-11-01T04:31:28Z

Use the new PathBase.scandir() method in PathBase.copy(), which greatly reduces the number of PathBase.stat() calls needed when copying. This also speeds up Path.copy(), which uses the superclass implementation.

Under the hood, we use directory entries to distinguish between files, directories and symlinks, and to retrieve a stat_result when reading metadata. This logic is extracted into a new pathlib._abc.CopierBase class, which helps reduce the number of underscore-prefixed support methods in the path interface. But it makes the patch a little large - sorry.

Issue: Expose os.DirEntry objects from pathlib #125413

Use the new `PathBase.scandir()` method in `PathBase.copy()`, which greatly reduces the number of `PathBase.stat()` calls needed when copying. This also speeds up `Path.copy()`, which inherits the superclass implementation. Under the hood, we use directory entries to distinguish between files, directories and symlinks, and to retrieve a `stat_result` when reading metadata. This logic is extracted into a new `pathlib._abc.CopierBase` class, which helps reduce the number of underscore-prefixed support methods in the path interface.

barneygale · 2024-11-01T23:37:29Z

Copying a directory of 100 empty files, this is about 10% faster when preserving metadata, and 5% faster without.

picnixz

Here's my nocturnal review just before going to bed!

Lib/pathlib/_abc.py

Lib/pathlib/_local.py

Lib/pathlib/_os.py

Misc/NEWS.d/next/Library/2024-11-01-04-21-26.gh-issue-125413.Z-jjZq.rst

Lib/pathlib/_abc.py

Co-authored-by: Bénédikt Tran <[email protected]>

barneygale · 2024-11-03T13:47:43Z

Thanks for the reviews, both! I'll wait to see if Adam has feedback before I merge.

barneygale · 2024-11-24T18:38:03Z

Gentle nudge @AA-Turner, thanks in advance :)

In case you didn't spot it, I'm planning to make scandir() private again after this PR lands. Thread here. The TL;DR is that it's not sufficiently motivated at the moment, but it might be if we make the pathlib ABCs public later on.

barneygale · 2024-12-05T21:46:50Z

Lib/pathlib/_abc.py

@@ -931,7 +975,7 @@ def move(self, target):
        """
        Recursively move this file or directory tree to the given destination.
        """
-        self._ensure_different_file(target)
+        target._copier.ensure_different_files(self, target)


This is wrong - target might be a string here.

Indeed, or a PathLike - could we use self._copier?

AA-Turner

Thank you for the reminder Barney.

I've no real concerns here, but I admit I'm not entirely sure why _CopierBase needs to exist, as it seems to reimplement some functionality of the Path hierarchy. It may be that the implementation without it would've been much longer, though.

Other than that, comments throughout.

A

AA-Turner · 2024-12-08T04:45:11Z

Lib/pathlib/_abc.py

+            try:
+                source_st = dir_entry.stat()
+            except AttributeError:
+                source_st = source.stat()


Are there valid non-None types for dir_entry that don't have stat? Otherwise a is None check is cheaper.

AA-Turner · 2024-12-08T04:54:52Z

Lib/pathlib/_abc.py

@@ -931,7 +975,7 @@ def move(self, target):
        """
        Recursively move this file or directory tree to the given destination.
        """
-        self._ensure_different_file(target)
+        target._copier.ensure_different_files(self, target)


Indeed, or a PathLike - could we use self._copier?

AA-Turner · 2024-12-08T05:02:58Z

Lib/pathlib/_local.py

+            try:
+                source = os.fspath(source)
+            except TypeError:
+                if not isinstance(source, PathBase):
+                    raise
+                CopierBase.copy_file(self, source, target, metadata_keys, dir_entry)


Do we permit types inheriting from pathlib._abc.PathBase but not also PurePath (or: not implementing __fspath__)? This seems a little strange at first look.

AA-Turner · 2024-12-08T05:12:18Z

Lib/pathlib/_local.py

+                    raise
+                CopierBase.copy_file(self, source, target, metadata_keys, dir_entry)
+            else:
+                copyfile(source, os.fspath(target))


Given the above exception handling for fspath, is target guaranteed to inherit from PurePath here?

AA-Turner · 2024-12-08T05:13:51Z

Lib/pathlib/_local.py

@@ -634,6 +672,12 @@ def _filter_trailing_slash(self, paths):
                path_str = path_str[:-1]
            yield path_str

+    def _join_dir_entry(self, dir_entry):
+        path_str = dir_entry.name if str(self) == '.' else dir_entry.path


Could we shortcut here rather than going via str(self)? The change to iterdir means that this is recalculated for each path in the directory being iterated over, rather than only once.

AA-Turner · 2024-12-08T05:17:42Z

Lib/pathlib/_abc.py

+            except IsADirectoryError as e:
+                if not target.exists():
+                    # Raise a less confusing exception.
+                    raise FileNotFoundError(
+                        f'Directory does not exist: {target}') from e
+                raise


Sanity checking -- does this directory error handling need to exist in copy_file? Can ensuring that source and target are files be handled as a precondition? No worries if 'yes', this just surprised me!

barneygale · 2025-02-21T18:39:10Z

Thanks for the feedback @AA-Turner, and my much-delayed response. I ended up going a different way with making os.DirEntry info available from pathlib: the scandir() method was replaced with a caching info attribute.

The spiritual successor to this PR here is here: #130238

In that PR, the new copy_file() function consolidates a bunch of copying logic from the _CopyWriter class, and uses source_path.info wherever possible to lean on cached scandir results.

I'll go through your feedback here and re-raise it on the other PR if it still applies.

Closing this PR. Sorry for the faff!

barneygale added performance Performance or resource usage topic-pathlib labels Nov 1, 2024

bedevere-app bot mentioned this pull request Nov 1, 2024

Expose os.DirEntry objects from pathlib #125413

Closed

Fix tests

fc40a79

barneygale marked this pull request as ready for review November 1, 2024 05:20

bedevere-app bot added the awaiting core review label Nov 1, 2024

Merge branch 'main' into pythongh-125413-copy

e688be8

barneygale requested review from AA-Turner and pfmoore November 1, 2024 21:12

pfmoore approved these changes Nov 1, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Nov 1, 2024

barneygale added 3 commits November 1, 2024 21:53

Speed up metadata handling

6f5ebb8

Speed up ensure_different_files()

09aff17

Speed up path construction when recursing

00abf8a

Fix missed Windows changes

3db0e18

picnixz reviewed Nov 2, 2024

View reviewed changes

Apply suggestions from code review

c4bd4a8

Co-authored-by: Bénédikt Tran <[email protected]>

barneygale mentioned this pull request Nov 3, 2024

GH-125413: pathlib.Path.copy(): get common metadata keys only once #125990

Closed

Merge branch 'main' into pythongh-125413-copy

69eb3c8

Merge branch 'main' into pythongh-125413-copy

7dd35b4

barneygale commented Dec 5, 2024

View reviewed changes

AA-Turner reviewed Dec 8, 2024

View reviewed changes

barneygale closed this Feb 21, 2025

barneygale mentioned this pull request Feb 21, 2025

GH-125413: Add private pathlib.Path method to write metadata #130238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-125413: pathlib: use `scandir()` to speed up `copy()` #126263

GH-125413: pathlib: use `scandir()` to speed up `copy()` #126263

barneygale commented Nov 1, 2024 •

edited by bedevere-app bot

Loading

barneygale commented Nov 1, 2024

picnixz left a comment

barneygale commented Nov 3, 2024

barneygale commented Nov 24, 2024

barneygale Dec 5, 2024

AA-Turner Dec 8, 2024

AA-Turner left a comment

AA-Turner Dec 8, 2024

AA-Turner Dec 8, 2024

AA-Turner Dec 8, 2024

AA-Turner Dec 8, 2024

AA-Turner Dec 8, 2024

AA-Turner Dec 8, 2024

barneygale commented Feb 21, 2025

GH-125413: pathlib: use scandir() to speed up copy() #126263

GH-125413: pathlib: use scandir() to speed up copy() #126263

Conversation

barneygale commented Nov 1, 2024 • edited by bedevere-app bot Loading

barneygale commented Nov 1, 2024

picnixz left a comment

Choose a reason for hiding this comment

barneygale commented Nov 3, 2024

barneygale commented Nov 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AA-Turner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barneygale commented Feb 21, 2025

GH-125413: pathlib: use `scandir()` to speed up `copy()` #126263

GH-125413: pathlib: use `scandir()` to speed up `copy()` #126263

barneygale commented Nov 1, 2024 •

edited by bedevere-app bot

Loading