Proposal for two micro-optimzations in RequestHandler.start function

[pfreixes]

Basically, I would like to work on two different micro-optimizations to speed up a bit, arround 5% and 10% overall, for the most costly function RequestHandler.start. I would like to gather your feedback before start working on them.

1. Handle most of the keepalive logic outside of the RequestHandler.start, and leave there only the none happy path - when the keep alive has to be canceled. This will save us to execute a few opcodes and many time syscalls. [1]

2. Get rid of the set_tcp_cork and set_tcp_nodelay operations and provide an API to be used by the internal layer that guarantees that a code path that touches any TCP configuration sets back the original values, this might be implemented using a context. [2]

I know that I might be missing something that would avoid implementing this two micro-optimizations, hence please feedback will be more than appreciate

[1] https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web_protocol.py#L476
[2] https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web_protocol.py#L426

[asvetlov]

1. Sorry, I don't follow your idea for keepalive optimization. Please elaborate.
2. set_tcp_cork and set_tcp_nodelay doesn't perform syscalls if nodelay is set (default). On other hand I'm inclining to drop cork/nodelay support at all. TCP connections should enable nodelay mode on connection initialization (modern asyncio does it already), no reason to touch the param. Especially taking into account HTTP/2 where single socket connection is shared between multiple streams.

[pfreixes]

Regarding the calls to the set_tcp_cork and set_tcp_nodelay, even though these do not perform any syscall the footprint that they have is not negligible, here an example of the cost of calling them

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
    96965    0.068    0.000    0.133    0.000 /home/ec2-user/aiohttp/http_writer.py:178(set_tcp_cork)
    96965    0.065    0.000    0.065    0.000 /home/ec2-user/aiohttp/http_writer.py:93(set_tcp_cork)
    96965    0.059    0.000    0.099    0.000 /home/ec2-user/aiohttp/http_writer.py:171(set_tcp_nodelay)
    96986    0.039    0.000    0.040    0.000 /home/ec2-user/aiohttp/http_writer.py:69(set_tcp_nodelay)

Compared to the cost of the RequestHandler.start function

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
    96985    1.424    0.000   13.597    0.000 /home/ec2-user/aiohttp/web_protocol.py:375(start)

Getting rid of this will give an increase of the performance of roughly 5%.

The only place where the TCP is tunned is in that is really used is in by the web_fileresponse [1], that might have totally sense - attention see that the finally restores the nodelay rather than do it over the cork option, how we could handle this breaking change taking into account that tune the TCP might be useful for some types of payload?

[1] https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web_fileresponse.py#L133

[asvetlov]

We can drop cork even in file response: if sendfile is not supported transport buffering should be enough .

[pfreixes]

Hidding in somehow the access to the TCP tunning methods giving to the API consistency .... or it is too much and overcomplicates and we believe that we won't fail?

[asvetlov]

Explicit support for NODELAY and CORK is really overcomplication.
I've added these methods but now I think it was mistake.

[pfreixes]

PayloadWriter is also used by the client, do you agree on get rid of the setters for the client and the webserver?

[fafhrd91]

if we can, we should remove cork and nodally api. it is broken right now for pipelined requests on server

[pfreixes]

Forget about

Handle most of the keepalive logic outside of the RequestHandler.start, and leave there only the none happy path - when the keep alive has to be canceled. This will save us to execute a few opcodes and many time syscalls. [1]

[asvetlov]

Fixed by #2612

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new