GH-125413: pathlib: use scandir() to speed up copy() - #126263
GH-125413: pathlib: use scandir() to speed up copy()#126263barneygale wants to merge 10 commits into
Conversation
Sorry, something went wrong.
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.
|
Copying a directory of 100 empty files, this is about 10% faster when preserving metadata, and 5% faster without. |
Sorry, something went wrong.
picnixz
left a comment
There was a problem hiding this comment.
Here's my nocturnal review just before going to bed!
Sorry, something went wrong.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
|
Thanks for the reviews, both! I'll wait to see if Adam has feedback before I merge. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| Recursively move this file or directory tree to the given destination. | ||
| """ | ||
| self._ensure_different_file(target) | ||
| target._copier.ensure_different_files(self, target) |
There was a problem hiding this comment.
This is wrong - target might be a string here.
Sorry, something went wrong.
There was a problem hiding this comment.
Indeed, or a PathLike - could we use self._copier?
Sorry, something went wrong.
AA-Turner
left a comment
There was a problem hiding this comment.
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
Sorry, something went wrong.
| try: | ||
| source_st = dir_entry.stat() | ||
| except AttributeError: | ||
| source_st = source.stat() |
There was a problem hiding this comment.
Are there valid non-None types for dir_entry that don't have stat? Otherwise a is None check is cheaper.
Sorry, something went wrong.
| Recursively move this file or directory tree to the given destination. | ||
| """ | ||
| self._ensure_different_file(target) | ||
| target._copier.ensure_different_files(self, target) |
There was a problem hiding this comment.
Indeed, or a PathLike - could we use self._copier?
Sorry, something went wrong.
| try: | ||
| source = os.fspath(source) | ||
| except TypeError: | ||
| if not isinstance(source, PathBase): | ||
| raise | ||
| CopierBase.copy_file(self, source, target, metadata_keys, dir_entry) |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| raise | ||
| CopierBase.copy_file(self, source, target, metadata_keys, dir_entry) | ||
| else: | ||
| copyfile(source, os.fspath(target)) |
There was a problem hiding this comment.
Given the above exception handling for fspath, is target guaranteed to inherit from PurePath here?
Sorry, something went wrong.
| yield path_str | ||
|
|
||
| def _join_dir_entry(self, dir_entry): | ||
| path_str = dir_entry.name if str(self) == '.' else dir_entry.path |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| except IsADirectoryError as e: | ||
| if not target.exists(): | ||
| # Raise a less confusing exception. | ||
| raise FileNotFoundError( | ||
| f'Directory does not exist: {target}') from e | ||
| raise |
There was a problem hiding this comment.
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!
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
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.