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.

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

Proposal for two micro-optimzations in RequestHandler.start function #2604

Closed
pfreixes opened this issue Dec 12, 2017 · 10 comments
Closed

Proposal for two micro-optimzations in RequestHandler.start function #2604

pfreixes opened this issue Dec 12, 2017 · 10 comments
Labels

Comments

@pfreixes
Copy link
Contributor

pfreixes commented Dec 12, 2017

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
Copy link
Member

  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
Copy link
Contributor Author

pfreixes commented Dec 13, 2017

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
Copy link
Member

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

@pfreixes
Copy link
Contributor Author

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
Copy link
Member

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

@pfreixes
Copy link
Contributor Author

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

@fafhrd91
Copy link
Member

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

@pfreixes
Copy link
Contributor Author

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
Copy link
Member

Fixed by #2612

@lock
Copy link

lock bot commented Oct 28, 2019

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

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants