Skip to content
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.

Sign up for GitHub

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

Jump to bottom

subprocess.communicate() should use a memoryview #63705

Closed
vstinner opened this issue Nov 5, 2013 · 15 comments
Closed

subprocess.communicate() should use a memoryview #63705

vstinner opened this issue Nov 5, 2013 · 15 comments
Labels
performance Performance or resource usage

Comments

@vstinner
Copy link
Member

vstinner commented Nov 5, 2013

BPO 19506
Nosy @gpshead, @pitrou, @vstinner, @serhiy-storchaka
Files
  • subprocess_memoryview-2.patch
  • test_sub.py
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2013-12-08.03:18:21.771>
created_at = <Date 2013-11-05.20:34:46.847>
labels = ['performance']
title = 'subprocess.communicate() should use a memoryview'
updated_at = <Date 2013-12-08.03:18:21.770>
user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2013-12-08.03:18:21.770>
actor = 'gregory.p.smith'
assignee = 'none'
closed = True
closed_date = <Date 2013-12-08.03:18:21.771>
closer = 'gregory.p.smith'
components = []
creation = <Date 2013-11-05.20:34:46.847>
creator = 'vstinner'
dependencies = []
files = ['32720', '32903']
hgrepos = []
issue_num = 19506
keywords = ['patch']
message_count = 15.0
messages = ['202240', '203449', '203450', '203451', '203462', '203475', '203478', '203479', '203484', '204792', '204797', '204800', '204903', '205508', '205509']
nosy_count = 7.0
nosy_names = ['gregory.p.smith', 'pitrou', 'vstinner', 'neologix', 'python-dev', 'sbt', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue19506'
versions = ['Python 3.4']
    @vstinner
    Copy link
    Member Author

    vstinner commented Nov 5, 2013

    The following code copies data, whereas the copy can be avoided using a memory view:

    chunk = self._input[self._input_offset:self._input_offset + _PIPE_BUF]
self._input_offset += os.write(key.fd, chunk)

    It should be replaced with:

    input_view = memoryview(self._input)
...
chunk = input_view[self._input_offset:self._input_offset + _PIPE_BUF]
self._input_offset += os.write(key.fd, chunk)

    This issue is a reminder for one of my comment of issue bpo-18923.

    @vstinner vstinner added the performance Performance or resource usage label Nov 5, 2013
    @vstinner
    Copy link
    Member Author

    Attached subprocess_memoryview.patch uses a memoryview() to avoid memory copies.

    The behaviour is undefined if two threads share the same Popen object and call the communicate() method at the same time. But I'm not sure that Popen is thread safe, it doesn't use any lock.

    @pitrou
    Copy link
    Member

    pitrou commented Nov 20, 2013

    Hi, why does your patch change listobject.c?

    @vstinner
    Copy link
    Member Author

    Hi, why does your patch change listobject.c?

    Oops, the change is unrelated (bpo-19578).

    @serhiy-storchaka
    Copy link
    Member

    What is a performance for small chunks?

    @vstinner
    Copy link
    Member Author

    What is a performance for small chunks?

    I have no idea. Does it matter?

    @serhiy-storchaka
    Copy link
    Member

    I don't know. But a function call have a cost.

    @vstinner
    Copy link
    Member Author

    New patch without the unwanted change in listobject.c.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Nov 20, 2013

    Serhiy Storchaka added the comment:

    I don't know. But a function call have a cost.

    I'm with Serhiy here.
    Writing a performance optimization without benchmark is generally a
    bad idea (I won't quote Knuth :-).

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Nov 30, 2013

    Could someone with a dual-core machine try the attached simplistic
    benchmark with and without Victor's patch?
    I can see some user-time difference with 'time' on my single-core
    machine, but I'm curious to see how this would affect things were both
    the parent and the child subprocess can run concurrently.

    @pitrou
    Copy link
    Member

    pitrou commented Nov 30, 2013

    On a quad-core machine:

    • without Victor's patch:
    $ time ./python test_sub.py 
0.39263957000002847

    real 0m0.521s
    user 0m0.412s
    sys 0m0.238s

    • with Victor's patch:
    $ time ./python test_sub.py 
0.3856174530001226

    real 0m0.516s
    user 0m0.404s
    sys 0m0.247s

    @serhiy-storchaka
    Copy link
    Member

    Best of 10 runs.

    Unpatched: 3.91057508099766
    Patched: 3.86466505300632

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Dec 1, 2013

    Impressive speedup :-)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 8, 2013

    New changeset 44948f5bdc12 by Gregory P. Smith in branch '3.3':
    Fixes issue bpo-19506: Use a memoryview to avoid a data copy when piping data
    http://hg.python.org/cpython/rev/44948f5bdc12

    New changeset 5379bba2fb21 by Gregory P. Smith in branch 'default':
    Fixes issue bpo-19506: Use a memoryview to avoid a data copy when piping data
    http://hg.python.org/cpython/rev/5379bba2fb21

    @gpshead
    Copy link
    Member

    gpshead commented Dec 8, 2013

    fwiw, this patch made sense. using test_sub.py on my dual-core amd64 box with an opt build is saw the times drop from ~0.90-0.97 to ~0.75-0.8. more speedup than i was really anticipating for this use case. it probably depends upon what the value of _PIPE_BUF is on your system.

    @gpshead gpshead closed this as completed Dec 8, 2013
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    performance Performance or resource usage
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @gpshead @vstinner @pitrou @serhiy-storchaka