[issue25156] shutil.copyfile should internally use os.sendfile when possible

[Berker Peksag]

Change by Berker Peksag :


--
superseder:  -> Efficient zero-copy for shutil.copy* functions (Linux, OSX and 
Win)

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

Closing as duplicate of #33671.

--
resolution:  -> duplicate
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[STINNER Victor]

STINNER Victor  added the comment:

Different implementation: bpo-33639.

--
nosy: +vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[Giampaolo Rodola']

Change by Giampaolo Rodola' :


--
nosy: +giampaolo.rodola

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

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/

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

Can this patch be merged, or is there something I can do to improve it?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[SilentGhost]

SilentGhost added the comment:

No further comments from me. I haven't run the test, but I trust it passes 
without any warnings.

--
nosy: +SilentGhost

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

Thank you SilentGhost for the second review on the v4 patch.

Attached is the v5 patch which hopefully is getting even better.

--
Added file: http://bugs.python.org/file41252/issue25156_v5.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

Here is a new patch, with changes suggested by SilentGhost and josh.rosenberg 
in the review.

--
Added file: http://bugs.python.org/file41243/issue25156_v4.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

Ping

A small patch, but a good performance improvement :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

Here is an updated patch that takes into account Martin's suggestions, both 
here and in the code review.

--
Added file: http://bugs.python.org/file40911/issue25156_v3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[R. David Murray]

R. David Murray added the comment:

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 :)

--
nosy: +gps

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

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

--
Added file: http://bugs.python.org/file40906/issue25156_v2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[Martin Panter]

Martin Panter added 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.

--
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[Martin Panter]

Martin Panter added the comment:

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 
 says it will raise 
EINVAL in this case. Maybe a test case would be good to prove this is still 
handled.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[Josh Rosenberg]

Josh Rosenberg added the comment:

Adding interested parties from earlier ticket.

--
nosy: +josh.r, martin.panter, r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

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)'
1 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)'
1 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

--
keywords: +patch
Added file: http://bugs.python.org/file40881/issue25156.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

desbma added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25156] shutil.copyfile should internally use os.sendfile when possible

[desbma]

New submission from desbma:

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.

--
components: Library (Lib)
messages: 250918
nosy: desbma
priority: normal
severity: normal
status: open
title: shutil.copyfile should internally use os.sendfile when possible
type: enhancement
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com