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 <10796600+picnixz@users.noreply.github.com>

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

 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()
+exceptAttributeError:
+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

 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)
+exceptTypeError:
+ifnotisinstance(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

 yieldpath_str

+def_join_dir_entry(self, dir_entry):
+path_str=dir_entry.nameifstr(self) =='.'elsedir_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

+exceptIsADirectoryErrorase:
+ifnottarget.exists():
+# Raise a less confusing exception.
+raiseFileNotFoundError(
+f'Directory does not exist: {target}') frome
+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-appbot 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-appbot added the awaiting core review label Nov 1, 2024

Merge branch 'main' intopythongh-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-appbot 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 <10796600+picnixz@users.noreply.github.com>

barneygale mentioned this pull request Nov 3, 2024
GH-125413: pathlib.Path.copy(): get common metadata keys only once #125990
Closed

Merge branch 'main' intopythongh-125413-copy
69eb3c8

Merge branch 'main' intopythongh-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

Uh oh!

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

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

Uh oh!

Conversation

barneygale commented Nov 1, 2024• edited by bedevere-app botLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barneygale commented Nov 1, 2024

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

barneygale commented Nov 3, 2024

Uh oh!

barneygale commented Nov 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AA-Turner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barneygale commented Feb 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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