Repository navigation
bpo-33671 / shutil.copyfile: use memoryview() with dynamic size on Windows #7681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 14 commits
820fc21
7c55be5
db6a9ad
090df6a
df2354f
4e2b9f8
dae995a
9f6cf1f
06d3e7a
8a3f88e
f47c011
c3166e1
89cd883
be40ae1
93ebc1f
9d93ce5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,15 +43,16 @@ | |
| except ImportError: | ||
| getgrnam = None | ||
|
|
||
| _WINDOWS = os.name == 'nt' | ||
| posix = nt = None | ||
| if os.name == 'posix': | ||
| import posix | ||
| elif os.name == 'nt': | ||
| elif _WINDOWS: | ||
| import nt | ||
|
|
||
| COPY_BUFSIZE = 1024 * 1024 if os.name == 'nt' else 16 * 1024 | ||
| COPY_BUFSIZE = 1024 * 1024 if _WINDOWS else 16 * 1024 | ||
| _HAS_SENDFILE = posix and hasattr(os, "sendfile") | ||
| _HAS_FCOPYFILE = posix and hasattr(posix, "_fcopyfile") # OSX | ||
| _HAS_FCOPYFILE = posix and hasattr(posix, "_fcopyfile") # macOS | ||
|
|
||
| __all__ = ["copyfileobj", "copyfile", "copymode", "copystat", "copy", "copy2", | ||
| "copytree", "move", "rmtree", "Error", "SpecialFileError", | ||
|
|
@@ -88,9 +89,9 @@ class _GiveupOnFastCopy(Exception): | |
| file copy when fast-copy functions fail to do so. | ||
| """ | ||
|
|
||
| def _fastcopy_osx(fsrc, fdst, flags): | ||
| def _fastcopy_fcopyfile(fsrc, fdst, flags): | ||
| """Copy a regular file content or metadata by using high-performance | ||
| fcopyfile(3) syscall (OSX). | ||
| fcopyfile(3) syscall (macOS). | ||
| """ | ||
| try: | ||
| infd = fsrc.fileno() | ||
|
|
@@ -169,7 +170,7 @@ def _fastcopy_sendfile(fsrc, fdst): | |
| offset += sent | ||
|
|
||
| def _copybinfileobj(fsrc, fdst, length=COPY_BUFSIZE): | ||
| """Copy 2 regular file objects open in binary mode.""" | ||
| """readinto()/memoryview() based variant of copyfileobj().""" | ||
| # Localize variable access to minimize overhead. | ||
| fsrc_readinto = fsrc.readinto | ||
| fdst_write = fdst.write | ||
|
|
@@ -179,28 +180,21 @@ def _copybinfileobj(fsrc, fdst, length=COPY_BUFSIZE): | |
| if not n: | ||
| break | ||
| elif n < length: | ||
| fdst_write(mv[:n]) | ||
| with mv[:n] as smv: | ||
| fdst.write(smv) | ||
| else: | ||
| fdst_write(mv) | ||
|
|
||
| def _is_binary_files_pair(fsrc, fdst): | ||
| return hasattr(fsrc, 'readinto') and \ | ||
| isinstance(fsrc, io.BytesIO) or 'b' in getattr(fsrc, 'mode', '') and \ | ||
| isinstance(fdst, io.BytesIO) or 'b' in getattr(fdst, 'mode', '') | ||
|
|
||
| def copyfileobj(fsrc, fdst, length=COPY_BUFSIZE): | ||
| """copy data from file-like object fsrc to file-like object fdst""" | ||
| if _is_binary_files_pair(fsrc, fdst): | ||
| _copybinfileobj(fsrc, fdst, length=length) | ||
| else: | ||
| # Localize variable access to minimize overhead. | ||
| fsrc_read = fsrc.read | ||
| fdst_write = fdst.write | ||
| while 1: | ||
| buf = fsrc_read(length) | ||
| if not buf: | ||
| break | ||
| fdst_write(buf) | ||
| # Localize variable access to minimize overhead. | ||
| fsrc_read = fsrc.read | ||
| fdst_write = fdst.write | ||
| while True: | ||
| buf = fsrc_read(length) | ||
| if not buf: | ||
| break | ||
| fdst_write(buf) | ||
|
|
||
| def _samefile(src, dst): | ||
| # Macintosh, Unix. | ||
|
|
@@ -215,7 +209,7 @@ def _samefile(src, dst): | |
| os.path.normcase(os.path.abspath(dst))) | ||
|
|
||
| def copyfile(src, dst, *, follow_symlinks=True): | ||
| """Copy data from src to dst. | ||
| """Copy data from src to dst in the most efficient way possible. | ||
|
|
||
| If follow_symlinks is not set and src is a symbolic link, a new | ||
| symlink will be created instead of copying the file it points to. | ||
|
|
@@ -224,7 +218,8 @@ def copyfile(src, dst, *, follow_symlinks=True): | |
| if _samefile(src, dst): | ||
| raise SameFileError("{!r} and {!r} are the same file".format(src, dst)) | ||
|
|
||
| for fn in [src, dst]: | ||
| file_size = 0 | ||
| for i, fn in enumerate([src, dst]): | ||
| try: | ||
| st = os.stat(fn) | ||
| except OSError: | ||
|
|
@@ -234,26 +229,36 @@ def copyfile(src, dst, *, follow_symlinks=True): | |
| # XXX What about other special files? (sockets, devices...) | ||
| if stat.S_ISFIFO(st.st_mode): | ||
| raise SpecialFileError("`%s` is a named pipe" % fn) | ||
| if _WINDOWS and i == 0: | ||
| file_size = st.st_size | ||
|
|
||
| if not follow_symlinks and os.path.islink(src): | ||
| os.symlink(os.readlink(src), dst) | ||
| else: | ||
| with open(src, 'rb') as fsrc, open(dst, 'wb') as fdst: | ||
| # Linux / Solaris | ||
| if _HAS_SENDFILE: | ||
| try: | ||
| _fastcopy_sendfile(fsrc, fdst) | ||
| return dst | ||
| except _GiveupOnFastCopy: | ||
| pass | ||
|
|
||
| if _HAS_FCOPYFILE: | ||
| # macOS | ||
| elif _HAS_FCOPYFILE: | ||
| try: | ||
| _fastcopy_osx(fsrc, fdst, posix._COPYFILE_DATA) | ||
| _fastcopy_fcopyfile(fsrc, fdst, posix._COPYFILE_DATA) | ||
| return dst | ||
| except _GiveupOnFastCopy: | ||
| pass | ||
| # Windows: for files >= 128 MiB in size we observe a | ||
| # considerable speedup by using a readinto()/memoryview() | ||
| # variant of copyfileobj(), see: | ||
| # https://github.com/python/cpython/pull/7160#discussion_r195162475 | ||
| elif _WINDOWS and file_size >= 128 * 1024 * 1024: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you mind to add a top-level private constant rather than an hardcoded value? (Would it make sense to make it somehow public? Or add an optional parameter...?)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is outdated, I modified code to always use the readinto() variant regardless of file size, see: #7160 (comment) |
||
| _copybinfileobj(fsrc, fdst) | ||
| return dst | ||
|
|
||
| _copybinfileobj(fsrc, fdst) | ||
| copyfileobj(fsrc, fdst) | ||
|
|
||
| return dst | ||
|
|
||
|
|
@@ -1147,7 +1152,7 @@ def disk_usage(path): | |
| used = (st.f_blocks - st.f_bfree) * st.f_frsize | ||
| return _ntuple_diskusage(total, used, free) | ||
|
|
||
| elif os.name == 'nt': | ||
| elif _WINDOWS: | ||
|
|
||
| __all__.append('disk_usage') | ||
| _ntuple_diskusage = collections.namedtuple('usage', 'total used free') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,10 @@ | ||
| :func:`shutil.copyfile`, :func:`shutil.copy`, :func:`shutil.copy2`, | ||
| :func:`shutil.copytree` and :func:`shutil.move` use platform-specific | ||
| fast-copy syscalls on Linux, Solaris and OSX in order to copy the file | ||
| more efficiently. All other platforms not using such technique will rely on a | ||
| faster :func:`shutil.copyfile` implementation using :func:`memoryview`, | ||
| :class:`bytearray` and | ||
| :meth:`BufferedIOBase.readinto() <io.BufferedIOBase.readinto>`. | ||
| Finally, :func:`shutil.copyfile` default buffer size on Windows was increased | ||
| from 16KB to 1MB. The speedup for copying a 512MB file is about +26% on Linux, | ||
| +50% on OSX and +38% on Windows. Also, much less CPU cycles are consumed | ||
| fast-copy syscalls on Linux, Solaris and macOS in order to copy the file | ||
| more efficiently. | ||
| On Windows :func:`shutil.copyfile` uses a bigger default buffer size (1 MiB | ||
| instead of 16 KiB) and if file size >= 128 MiB a :func:`memoryview`-based | ||
| variant of :func:`shutil.copyfileobj` is used. | ||
| The speedup for copying a 512MiB file is about +26% on Linux, +50% on macOS and | ||
| +40% on Windows. Also, much less CPU cycles are consumed. | ||
| (Contributed by Giampaolo Rodola' in :issue:`25427`.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both options are always exclusive? macOS might have sendfile() in the future, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now outdated: I updated code to always use fcopyfile on OSX and leave sendfile() alone. This way we avoid invoking sendfile() on OSX which would fail anyway.