classification
Title: shutil.copyfile should internally use os.sendfile when possible
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: duplicate
Dependencies: Superseder: Efficient zero-copy for shutil.copy* functions (Linux, OSX and Win)
View: 33671
Assigned To: Nosy List: SilentGhost, desbma, giampaolo.rodola, gps, josh.r, martin.panter, r.david.murray, vstinner
Priority: normal Keywords: patch

Created on 2015-09-17 20:08 by desbma, last changed 2018-07-27 13:26 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
issue25156.patch desbma, 2015-10-28 23:41 First patch review
issue25156_v2.patch desbma, 2015-10-30 22:53 review
issue25156_v3.patch desbma, 2015-10-31 16:13 review
issue25156_v4.patch desbma, 2015-12-05 00:14 review
issue25156_v5.patch desbma, 2015-12-05 21:48 review
Messages (21)
msg250918 - (view) Author: desbma (desbma) * Date: 2015-09-17 20:08
This is related to issue25063 (https://bugs.python.org/issue25063).

Trying to use sendfile internally in shutil.copyfileobj was considered risky because of special Python files that expose a file descriptor but wrap it to add special behavior (eg: GzipFile).

I believe such risk does not exist for shutil.copyfile, and it would be possible to use sendfile if available.
msg251246 - (view) Author: desbma (desbma) * Date: 2015-09-21 18:16
Additional advantage of calling sendfile from shutil.copyfile: other fonctions in shutil module would automatically benefit from the use of senfile because they call copyfile directly (copy, copy2) or indirectly (copytree).

So for example, the performance of shutil.copytree should be improved for free for directory trees containing big files.
msg253643 - (view) Author: desbma (desbma) * Date: 2015-10-28 23:41
Thoughts anyone?
Here is a patch that implements the change.

My tests show a 30-40% performance improvement for 128KB-512MB single file copy:

128 KB file copy:

$ dd if=/dev/urandom of=/tmp/f1 bs=1K count=128

Without the patch:
$ ./python -m timeit -s 'import shutil; p1 = "/tmp/f1"; p2 = "/tmp/f2"' 'shutil.copyfile(p1, p2)'
10000 loops, best of 3: 109 usec per loop

With the patch:
$ ./python -m timeit -s 'import shutil; p1 = "/tmp/f1"; p2 = "/tmp/f2"' 'shutil.copyfile(p1, p2)'
10000 loops, best of 3: 75.7 usec per loop

--------
8 MB file copy:

$ dd if=/dev/urandom of=/tmp/f1 bs=1M count=8

Without the patch:
$ ./python -m timeit -s 'import shutil; p1 = "/tmp/f1"; p2 = "/tmp/f2"' 'shutil.copyfile(p1, p2)'
100 loops, best of 3: 4.99 msec per loop

With the patch:
$ ./python -m timeit -s 'import shutil; p1 = "/tmp/f1"; p2 = "/tmp/f2"' 'shutil.copyfile(p1, p2)'
100 loops, best of 3: 3.03 msec per loop

--------
512 MB file copy:

$ dd if=/dev/urandom of=/tmp/f1 bs=1M count=512

Without the patch:
$ ./python -m timeit -s 'import shutil; p1 = "/tmp/f1"; p2 = "/tmp/f2"' 'shutil.copyfile(p1, p2)'
10 loops, best of 3: 305 msec per loop

With the patch:
$ ./python -m timeit -s 'import shutil; p1 = "/tmp/f1"; p2 = "/tmp/f2"' 'shutil.copyfile(p1, p2)'
10 loops, best of 3: 178 msec per loop
msg253648 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2015-10-29 00:51
Adding interested parties from earlier ticket.
msg253649 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-29 01:18
I’ve never used sendfile() nor shutil.copyfile(), but my immediate reaction is maybe we need a backup plan if os.sendfile() is available but not supported in the circumstances. E.g. if it is practical to use copyfile() to copy from a named socket in the filesystem, the Linux man page <http://man7.org/linux/man-pages/man2/sendfile.2.html> says it will raise EINVAL in this case. Maybe a test case would be good to prove this is still handled.
msg253650 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-29 01:24
Also, the os.sendfile() doc suggests that some platforms only support writing to sockets, so I definitely think a backup plan is needed.
msg253669 - (view) Author: desbma (desbma) * Date: 2015-10-29 09:36
Thanks for the comment.

> Also, the os.sendfile() doc suggests that some platforms only support writing to sockets, so I definitely think a backup plan is needed.

You are right, the man page clearly says:
> Applications may wish to fall back to read(2)/write(2) in the case
> where sendfile() fails with EINVAL or ENOSYS.

I will improve the code and add tests for conditions where sendfile fails.

I have tested it manually, but I will also add a test with a copy of a file > 4GB (it causes several calls to sendfile).
msg253701 - (view) Author: desbma (desbma) * Date: 2015-10-29 23:25
I played a bit with Unix domain sockets, and it appears you can not open them like a file with open().
So they do no work with the current implementation of shutil.copyfile anyway.
msg253767 - (view) Author: desbma (desbma) * Date: 2015-10-30 22:53
Here is an improved patch with the following changes:
* Fallback to copyfileobj if sendfile fails with errno set to EINVAL or ENOSYS 
* Add a test for > 4GB file
msg253778 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-31 07:36
I left a few comments. But it might be good if someone more familiar with the APIs could review this.

Have you seen the socket.sendfile() implementation? It’s a bit of a mess, and the circumstances are slightly different, but it may offer partial inspiration. It seems a shame to have two separate high-level sendfile() wrappers, but I guess the use cases are just a little too far apart to be worth sharing much code.

For the test case, it may be worth skipping the test if you run out of disk space. Similar to test_mmap and test_largefile.
msg253781 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-31 08:08
Also, man pages for Free BSD and OS X (where writing to a disk file is not supported) say it raises:

* ENOTSUP if “the ‘fd’ argument does not refer to a regular file”
* EBADF if “the ‘s’ argument is not a valid socket descriptor”
* ENOTSOCK if “the ‘s’ argument is not a socket”
* EOPNOTSUPP if “the file system for descriptor fd does not support sendfile()”

It is not clear what the priority of these errors is, so it might be safest to catch them all. But I wouldn’t catch any arbitrary OSError, because you may end up doing weird double copying or something for an out-of-space error.
msg253790 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-10-31 14:29
I'm not at all sure this is worth the maintenance burden at this point in time.  So let's say I'm -0.5.  I agree that a review and opinion by someone more familiar with the API would be best.  I'm adding gps as nosy since this feels like the kind of performance improvement he might find interesting, so if he thinks it is worth it I'll change my vote :)
msg253799 - (view) Author: desbma (desbma) * Date: 2015-10-31 16:13
Here is an updated patch that takes into account Martin's suggestions, both here and in the code review.
msg255769 - (view) Author: desbma (desbma) * Date: 2015-12-02 20:13
Ping

A small patch, but a good performance improvement :)
msg255908 - (view) Author: desbma (desbma) * Date: 2015-12-05 00:14
Here is a new patch, with changes suggested by SilentGhost and josh.rosenberg in the review.
msg255981 - (view) Author: desbma (desbma) * Date: 2015-12-05 21:48
Thank you SilentGhost for the second review on the v4 patch.

Attached is the v5 patch which hopefully is getting even better.
msg255983 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2015-12-05 21:56
No further comments from me. I haven't run the test, but I trust it passes without any warnings.
msg257345 - (view) Author: desbma (desbma) * Date: 2016-01-02 11:55
Can this patch be merged, or is there something I can do to improve it?
msg259808 - (view) Author: desbma (desbma) * Date: 2016-02-07 23:03
If anyone is interested, I have created a package to monkey patch shutil.copyfile to benefit from sendfile, similarly to the last patch, but it also works on older Python versions down to 2.7.

PyPI link: https://pypi.python.org/pypi/pyfastcopy/
msg317628 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-05-24 22:03
Different implementation: bpo-33639.
msg322489 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2018-07-27 13:13
Closing as duplicate of #33671.
History
Date User Action Args
2018-07-27 13:26:04berker.peksagsetsuperseder: Efficient zero-copy for shutil.copy* functions (Linux, OSX and Win)
2018-07-27 13:13:35giampaolo.rodolasetstatus: open -> closed
resolution: duplicate
messages: + msg322489

stage: patch review -> resolved
2018-05-24 22:03:49vstinnersetnosy: + vstinner
messages: + msg317628
2018-05-19 11:25:56giampaolo.rodolasetnosy: + giampaolo.rodola
2016-02-07 23:03:57desbmasetmessages: + msg259808
2016-01-02 11:55:27desbmasetmessages: + msg257345
2015-12-05 21:56:29SilentGhostsetnosy: + SilentGhost
messages: + msg255983
2015-12-05 21:48:05desbmasetfiles: + issue25156_v5.patch

messages: + msg255981
2015-12-05 00:14:21desbmasetfiles: + issue25156_v4.patch

messages: + msg255908
2015-12-02 20:13:59desbmasetmessages: + msg255769
2015-10-31 16:13:50desbmasetfiles: + issue25156_v3.patch

messages: + msg253799
2015-10-31 14:29:39r.david.murraysetnosy: + gps
messages: + msg253790
2015-10-31 08:08:00martin.pantersetmessages: + msg253781
2015-10-31 07:36:33martin.pantersetmessages: + msg253778
2015-10-30 22:53:31desbmasetfiles: + issue25156_v2.patch

messages: + msg253767
2015-10-29 23:25:28desbmasetmessages: + msg253701
2015-10-29 09:36:16desbmasetmessages: + msg253669
2015-10-29 01:24:17martin.pantersetmessages: + msg253650
stage: patch review
2015-10-29 01:18:13martin.pantersetmessages: + msg253649
2015-10-29 00:51:00josh.rsetnosy: + r.david.murray, martin.panter, josh.r
messages: + msg253648
2015-10-28 23:41:24desbmasetfiles: + issue25156.patch
keywords: + patch
messages: + msg253643
2015-09-21 18:16:17desbmasetmessages: + msg251246
2015-09-17 20:08:05desbmacreate