do we need http pipelining?

[fafhrd91]

@asvetlov @kxepal thoughts?

[asvetlov]

Not sure.
On one hand why not if we could support it easy.
On other hand if pipelining requires extra steps from users they never do it.

[fafhrd91]

removing pipelining would simplify request handling flow. (some speedup is expected too)
pipelining requires read/write ordering. I doubt pipelining is in use, especially for python servers.

[fafhrd91]

on other hand pipelining help in synthetic benchmarks

[kxepal]

I'm not sure about as well. If HTTP/1 pipelining support cause maintenance issues, slows down general HTTP usage and gives only profit in synthetic benchmark, I think there is nothing much to think about.

It's pretty cool, but not very popular feature for HTTP/1, but it's a core one of HTTP/2. May be move it HTTP/2 implementation if any will happens?

Btw, do we know any aiohttp usage with pipelining in the wild?

[fafhrd91]

Pipelining gives nice 25% boost in synthetic tests :)

[socketpair]

https://medium.freecodecamp.org/million-requests-per-second-with-python-95c137af319

[fafhrd91]

how much code does it actually run? add business logic written in python and some logging, and this thing would show same speed as any other python framework.

[fafhrd91]

I added pipelining because of benchmarks :)

[samuelcolvin]

I think pipelining is only really used in tests. The mistake it tests/benchmarks allowing it/being too simple.

For me, remove if it makes code simpler or provides any other speedups.

[pfreixes]

I have a proposal for that.

Right now Aiohttp implements HTTP pipelining but this feature is not fully enabled - correct me if Im wrong @fafhrd91. The default concurrence per connection [1] does not allow Aiohttp to process more than one request at a time.

Having this scenario and taking into account the complexity that this feature adds in terms of code and the low usage in production systems, almost in microservices architectures, I would propose to reduce the current HTTP pipelining to a more naive implementation that does not support concurrence in the same connection.

Why ?

1. Reduce the codebase and as a result the performance footprint.
2. Give still support for HTTP pipelining for the none happy path and meeting the HTTP 1.1 requirements.

How ?

1. The first PR, just getting rid of the max_concurrent_handlers parameter and removing all of the code that preserves the arrival order and allows Aiohttp have more than one concurrent handler executing at a time.

2. The second PR, reduce if its possible the number of layers needed by request to handle the output.

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

[fafhrd91]

Pipelining is disabled because of benchmarks. On primitive benchmarks pipelining costs about 4-7%

I am fine with removing pipelining, but pipelined request still should be supported with concurrency 1

[pfreixes]

Oks, once the pipelining has been simplified using a naive implementation we can move forward to the next steps. @fafhrd91 you suggested that we might get rid of the StreamWriter once we have removed the complexity under the hood, my suggestion heads to remove the big fish, the PayloadWriter.

Right now Aiohttp needs at least the following three layers to handle the response: an instance of StreamWriter, an instance of PayloadWriter and finally an instance of StreamResponse, derivates of the last two ones are allowed. The idea is to reduce these three layers to just the StreamResponse one. My gut feeling says that it's doable, and that would be done making the following changes:

1. Get rid of the superfluous StreamWriter and turn the TCP tunning operations as simple functions.
2. Create an AbstractResponse as a replacement of the AbstractPayloadWriter specifying the functions that must be implemented. Basically, write methods.
3. Move the current logic of the write methods that are in the PayloadWriter to the current write methods of StreamResponse.
4. The StreamResponse - once is called the prepare method - will use the transport rather than use the PaylodWriter.
5. Derivates as web file response u others must implement only one derivate of the StreamResponse class.

This would be the sketch more or less, surely I will face problems that right now I cant see them. Anyway, thoughts @fafhrd91 and @asvetlov ?

[fafhrd91]

I do not remember all details, I wrote this code year ago. If you can simplify all three layers to only SessionResponse, that would be great. Some notes: * http_xxx.py modules are reused by client and server * server file response * stream write is just wrapper for transport, but payload write is actual data encoder

…

Sent from my iPhone

On Dec 28, 2017, at 1:17 AM, Pau Freixes ***@***.***> wrote: Oks, once the pipelining has been simplified using a naive implementation we can move forward to the next steps. @fafhrd91 you suggested that we might get rid of the StreamWriter once we have removed the complexity under the hood, my suggestion heads to remove the big fish, the PayloadWriter. Right now Aiohttp needs at least the following three layers to handle the response an instance of StreamWriter, an instance of PayloadWriter and finally an instance of StreamResponse, also derivates of the last two ones are allowed. The idea is to reduce these three layers to just the StreamResponse one. My gut feeling says that it's doable that would be done making the following changes: Get rid of the superfluous StreamWriter and turn the TCP tunning operations as simple functions. Create an AbstractResponse as a replacement of the AbstractPayloadWriter specifying the functions that must be implemented. Basically, write methods. Move the current logic of the write methods that are in the PayloadWriter to the current write methods of StreamResponse. The StreamResponse - once is called the prepare method - will use the transport rather than use the PaylodWriter. Derivates as web file response u others must implement only one derivate of the StreamResponse class. This would be the sketch more or less, surely I will face problems that right now I cant see them. Anyway, thoughts @fafhrd91 and @asvetlov ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[asvetlov]

+1
I support removal of extra indirection levels but we should look on PR with implementation first.
If simplification will require backward incompatible changes -- let's discuss every break separately.

[fafhrd91]

I don’t think any breakage will require

[asvetlov]

I hope so

[pfreixes]

Finally, I decided to go a bit slower taking into account that I could end up having something almost impossible - to hard - to review and also to make my process to change all of this with small iterations that will help us definitely.

So next MR just get rids the current legacy class StreamWriter as @fafhrd91 proposed.

#2651

[pfreixes]

For now, Im gonna "give up" at that point. Meaning that I won't go deeper to refactor more code, the reason is basically it is getting harder and harder and I need some time for rest regarding this subject.

Beside of that, don't you believe that this issue can be closed? Taking into account that the pipelining support has been maintained but making it simpler having the same expectations.

[asvetlov]

Agree

[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