bpo-41279: Add StreamReaderBufferedProtocol #21446

tontinton · 2020-07-11T17:17:39Z

https://bugs.python.org/issue41279

I got way better performance on await reader.read() using this branch on linux (check out the chart on the server.py script's comments).

The way I tested was writing a server / client:
server.py:

import asyncio
import contextlib
import time


async def client_connected(reader, writer):
    start = time.time()

    with contextlib.closing(writer):
        for i in range(1000):
            # the more this parameter's distance from 65536 is greater the better the performance global_buffer gives
            # On linux:
            # 65536 * 2 -> Gives about 160% better performance
            # 4096 -> Gives about 155% better performance
            # 65536 -> Gives about 125% better performance

            # On windows using 65536 gives the same performance for some reason, which is interesting
            # But any other value gives a bit better performance, for example 65536 * 2 gives about 120% better performance
            await reader.read(65536 * 2)

    print(f'{time.time() - start}')


async def main():
    server = await asyncio.start_server(client_connected, '127.0.0.1', 8888, global_buffer=True)

    addr = server.sockets[0].getsockname()
    print(f'Serving on {addr}')

    async with server:
        await server.serve_forever()


if __name__ == "__main__":
    asyncio.run(main())

client.py:

import asyncio
import contextlib


async def flood(ip, port):
    message = b'A' * 1024 * 64  # tweak this parameter as much as you want
    reader, writer = await asyncio.open_connection(ip, port)
    with contextlib.closing(writer):
        while True:
            writer.write(message)
            await writer.drain()


if __name__ == "__main__":
    asyncio.run(flood('127.0.0.1', 8888))

1st1 · 2020-07-16T21:27:00Z

Lib/asyncio/proactor_events.py

Do we have proper functional tests that hit this branch? If not we need to add them.

You can probably copy some tests from the uvloop project (as I own the project there will be no licensing issues even if it's a literal copy)

I am thinking of doing parameterize on all the appropriate test in test_proactor_events.py.

That way we can use the already existing tests on both Protocol and BufferedProtocol

Ok I did this using the parameterize function, it did made me write a bit of ugly code I want you to look at.

Check out what I did in the test adding commit.

By the way shouldn't I add the same tests for UnixReadPipeTransportTests?

Ok I added the tests to UnixReadPipeTransportTests as well using the same patches

Ok I did this using the parameterize function, it did made me write a bit of ugly code I want you to look at.

Yeah, let's try to simplify it. See my other comment.

By the way shouldn't I add the same tests for UnixReadPipeTransportTests?

I guess since we're updating _UnixReadPipeTransport we should.

Yeah I simplified it a lot

mpaolini · 2020-07-16T22:29:59Z

Lib/asyncio/proactor_events.py

Suggested change

except (SystemExit, KeyboardInterrupt):

raise

the two llines can be removed

mpaolini · 2020-07-17T09:11:50Z

Lib/asyncio/proactor_events.py

Suggested change

return

just a small change ;)

bedevere-bot · 2020-08-08T09:38:16Z

Thanks for making the requested changes!

@1st1, @asvetlov: please review the changes made to this pull request.

tontinton · 2021-10-18T14:00:25Z

Hey, just remembered that I did this, bumping! :)

gst · 2022-07-07T14:21:36Z

Lib/test/test_asyncio/test_proactor_events.py

why this double pause_reading() above and then resume_reading() here ?

and same question for below in test_pause_resume_reading.

gst · 2022-07-07T14:27:16Z

Lib/test/test_asyncio/test_proactor_events.py

before the _run_once() would not it be better to assert that the msg is not in the _mock_call_args_list of the mock object ?

and same below for the others data_received.assert_called_with following a _run_once().

I don't understand which mock object you are talking about, and I don't see any other test reading _mock_call_args_list.

Can you elaborate?

something like:

for msg in msgs[:2]: self.assertFalse(unittest.mock.call(msg) in self.protocol.data_received.mock_calls) self.loop._run_once() self.protocol.data_received.assert_called_with(msg)

to ensure that each msg is effectively processed during the _run_once() call.

Oh I see, but protocol's buffer_updated (data_received is only in a regular stream object, not buffered) is not a mock, it's an actual function, so no mock_calls field.

This class gets better performance as BufferedProtocol uses read_into into a buffer allocated before, instead of allocating a new buffer each time read is called.

…pythonGH-21446) The transport did not know how to use the proper api exported by BufferedProtocol. Added a new callback function that calls getbuffer() and buffer_updated() instead of data_received() when the protocol given to it is of type BufferedProtocol. This is exactly the same way _SelectorSocketTransport handles a BufferedProtocol.

…port (pythonGH-21446) In the __init__ function if the protocol is of instance BufferedProtocol instead of creating a buffer object, we call get_buffer on the protocol to get its buffer. In addition _loop_reading now calls _data_received as soon as there is actual data instead of calling only after adding a recv_into event. The reason for this change is because read_into could call it's callback immediatly meaning overriding the data on the buffer before we actually call _data_received on it, which fixes the potential issue of missed data.

tontinton · 2022-07-08T15:07:41Z

I'll fix the ssl tests sometime this weekend

arhadthedev · 2022-07-08T15:58:04Z

@tontinton You can turn your PR into a draft and not worry about timeframes.

…pythonGH-21446) When calling set_protocol to change the protocol you can now change the type of the protocol from BufferedProtocol to Protocol or vice versa. start_tls needed this feature as it could read into a buffered protocol at first and then change the protocol to SSLProto which is a regular protocol.

…-21446)

…thonGH-21446)

tontinton requested review from 1st1 and asvetlov as code owners July 11, 2020 17:17

the-knights-who-say-ni added the CLA signed label Jul 11, 2020

bedevere-bot added the awaiting review label Jul 11, 2020

tontinton changed the title ~~Fix issue 41279~~ bpo-41273: Convert StreamReaderProtocol to a BufferedProtocol Jul 11, 2020

tontinton changed the title ~~bpo-41273: Convert StreamReaderProtocol to a BufferedProtocol~~ bpo-41279: Convert StreamReaderProtocol to a BufferedProtocol Jul 11, 2020

tontinton force-pushed the fix-issue-41279 branch 2 times, most recently from d594f7a to af23da7 Compare July 11, 2020 17:38

tontinton changed the title ~~bpo-41279: Convert StreamReaderProtocol to a BufferedProtocol~~ bpo-41279: Convert StreamReaderProtocol to a BufferedProtocol for increased performance Jul 11, 2020

tontinton force-pushed the fix-issue-41279 branch from af23da7 to c533f08 Compare July 14, 2020 20:49

tontinton force-pushed the fix-issue-41279 branch from c533f08 to 806b335 Compare July 14, 2020 22:28

tontinton force-pushed the fix-issue-41279 branch from 806b335 to 62da6f0 Compare July 14, 2020 22:41

tontinton force-pushed the fix-issue-41279 branch from 62da6f0 to ac1418d Compare July 14, 2020 22:54

tontinton force-pushed the fix-issue-41279 branch from ac1418d to 71ea0a6 Compare July 15, 2020 08:04

1st1 reviewed Jul 16, 2020

View reviewed changes

mpaolini reviewed Jul 16, 2020

View reviewed changes

tontinton force-pushed the fix-issue-41279 branch from 71ea0a6 to 2a4a9eb Compare July 16, 2020 23:30

mpaolini reviewed Jul 17, 2020

View reviewed changes

tontinton force-pushed the fix-issue-41279 branch from 2a4a9eb to 38704db Compare July 17, 2020 10:45

bedevere-bot added the awaiting changes label Aug 3, 2020

tontinton requested a review from asvetlov August 5, 2020 16:10

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 8, 2020

bedevere-bot requested a review from 1st1 August 8, 2020 09:38

tontinton mannequin mentioned this pull request Apr 10, 2022

asyncio: proactor read transport: use recv_into instead of recv #85445

Closed

gst reviewed Jul 7, 2022

View reviewed changes

tontinton force-pushed the fix-issue-41279 branch from 1de298c to ea382bd Compare July 8, 2022 13:21

tontinton added 3 commits July 8, 2022 16:40

bpo-41279: Add StreamReaderBufferedProtocol (pythonGH-21446)

9a65cfe

This class gets better performance as BufferedProtocol uses read_into into a buffer allocated before, instead of allocating a new buffer each time read is called.

tontinton force-pushed the fix-issue-41279 branch from ea382bd to d0dcc84 Compare July 8, 2022 13:40

tontinton force-pushed the fix-issue-41279 branch from d0dcc84 to 35771ff Compare July 8, 2022 19:40

tontinton added 3 commits July 8, 2022 22:41

bpo-41279: Add BufferedProtocol to proactor transport tests (pythonGH…

9e81f1a

…-21446)

bpo-41279: Add BufferedProtocol to unix read pipe transport tests (py…

3dc4d18

…thonGH-21446)

tontinton force-pushed the fix-issue-41279 branch from 35771ff to 3dc4d18 Compare July 8, 2022 19:42

ezio-melotti removed the CLA signed label Jul 13, 2022

tontinton mannequin mentioned this pull request Apr 10, 2022

Add a StreamReaderBufferedProtocol #85451

Open

msoxzw mentioned this pull request Jan 28, 2023

asyncio tcp transport on Windows reads bytearray instead of bytes #99941

Closed

Uh oh!

bpo-41279: Add StreamReaderBufferedProtocol #21446

Are you sure you want to change the base?

bpo-41279: Add StreamReaderBufferedProtocol #21446

Uh oh!

Conversation

tontinton commented Jul 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 8, 2020

Uh oh!

tontinton commented Oct 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tontinton commented Jul 8, 2022

Uh oh!

arhadthedev commented Jul 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

tontinton commented Jul 11, 2020 •

edited

Loading

tontinton Jul 16, 2020 •

edited

Loading

tontinton Jul 17, 2020 •

edited

Loading

tontinton Jul 8, 2022 •

edited

Loading