Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.

Speedup readexactly#395

Closed
socketpair wants to merge 1 commit into
python:masterfrom
socketpair:exactly_speed
Closed

Speedup readexactly#395
socketpair wants to merge 1 commit into
python:masterfrom
socketpair:exactly_speed

Conversation

@socketpair

Copy link
Copy Markdown

See #394

@socketpair

Copy link
Copy Markdown
Author

I still not tested performance. Please be patient, or help me with that :)

Comment thread asyncio/streams.py Outdated
current_len = len(self._buffer)

if current_len >= n:
break

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while len(self._buffer) < n:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@socketpair

Copy link
Copy Markdown
Author

#394 should be fixed with that. Will benchmark after some time.

@1st1

1st1 commented Sep 15, 2016

Copy link
Copy Markdown
Member

@socketpair I can merge this PR in 3.6 today/tomorrow if you can show that it improves performance.

Comment thread asyncio/streams.py Outdated
yield from self._wait_for_data('readexactly')

data = bytes(memoryview(self._buffer)[:n])
del self._buffer[:n]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In non-cpython implementations, destructing of memoryview may be postponed, so cutting buffer may fail. In order to fix that, we should call memoryview_obj.release()...

Not sure if it is actual.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right, I think.
I wonder if there is more simple and safe API.

@socketpair socketpair Sep 22, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To overcome that, it is require to make two separate variables: i.e.

memview1 = memoryview(self._buffer)
memview2 = memview1[:n]
data = bytes(memview2)
memview2.release()
memview1.release()

So, I decide to eliminate memoryview...Yes it should be faster, but....

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should watch your language.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I have fixed comment.

@socketpair

socketpair commented Sep 22, 2016

Copy link
Copy Markdown
Author

I have benchmarked that.
8.72 on master branch
6.65 on this branch.

program:

#!/usr/bin/env python3.5

import asyncio
import os
import time

async def action(rs, chunksize):
    try:
        while True:
            await rs.readexactly(chunksize)
    except asyncio.IncompleteReadError:
        pass

async def writer(ws, bufsize):
    ws.write(b'x' * bufsize)
    await ws.drain()
    ws.close()

async def amain():
    bufsize = 30 * 1024 * 1024
    chunksize = 16

    handler = lambda rs, ws: writer(ws, bufsize)
    server = await asyncio.start_server(handler, '127.0.0.1', 0)
    addr = server.sockets[0].getsockname()
    (rs, ws) = await asyncio.open_connection(*addr)
    server.close()
    await server.wait_closed()
    # now writing part is running "in background". TODO: wait it completion somehow

    await asyncio.sleep(3)

    a = time.monotonic()
    await action(rs, chunksize)
    b = time.monotonic()
    ws.close()
    print('chunk size={}: {:.2f} seconds.'.format(
        chunksize,
        b - a,
    ))


loop = asyncio.get_event_loop()
loop.run_until_complete(amain())

@socketpair

Copy link
Copy Markdown
Author

Adding memoryview slowers down (!) so, it is good not to use memoryview before copying in that place.

@socketpair

Copy link
Copy Markdown
Author

@gvanrossum @methane @1st1

Any feedback ?

Comment thread asyncio/streams.py

yield from self._wait_for_data('readexactly')

if len(self._buffer) == n:

@socketpair socketpair Sep 26, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really frequent case for protocols like this: http://www.postfix.org/socketmap_table.5.html where we expect a string with known length, data after which is not expected.

(Did not benchmarked if this really speedup)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, I want to add helper C function for "slicing bytes from bytearray".
And I hope it will be part of Python 3.7, (e.g. bytes.frombuffer(buff, start=0, end=None)).

@methane

methane commented Sep 27, 2016

Copy link
Copy Markdown
Member

lgtm

@socketpair

Copy link
Copy Markdown
Author

@1st1 @gvanrossum ping

@1st1

1st1 commented Oct 3, 2016

Copy link
Copy Markdown
Member

I'll commit this later today.

@1st1

1st1 commented Oct 5, 2016

Copy link
Copy Markdown
Member

Committed by hand in b0e7438. Thanks!

@1st1 1st1 closed this Oct 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants