scylladb / seastar

High performance server-side application framework
http://seastar.io
Apache License 2.0
8.38k stars 1.55k forks source link

Coroutinize http client code #2429

Open xemul opened 2 months ago

xemul commented 2 months ago

The code looks nicer and with much less indentation depth that way

xemul commented 2 months ago

upd:

gleb-cloudius commented 2 months ago

As a general note when I tried to co-routinize cql server in Scylla I did not find a way to do so without introducing more allocations when requests are pipelined. With continuations if we read more than one request from a socket all of them can be processed without allocating continuations since futures will be ready. With co-routins we always allocate.

avikivity commented 2 months ago

With [[clang::coro_elide_safe]] nested coroutines can share the coroutine frame. However it only arrives in clang 20 and isn't standardized.

nyh commented 2 months ago

Hi @xemul,

When you created a 28-patch series which must have taken you many many hours to create, please spend another 10 minutes trying to explain your motivation for doing this. Was it just the generic "coroutines are nicer" sentiment that we all shared, or something specific which bothered you?

Also, I see a lot of discussion regarding the lower performance of the coroutine code, so I wonder if you made an attempt to benchmark the http server somehow to see if there's anything we need to worry about.

xemul commented 2 months ago

Hi @xemul,

When you created a 28-patch series which must have taken you many many hours to create, please spend another 10 minutes trying to explain your motivation for doing this. Was it just the generic "coroutines are nicer" sentiment that we all shared, or something specific which bothered you?

No, nothing specific. It just looks much nicer and simpler this way.

Also, I see a lot of discussion regarding the lower performance of the coroutine code, so I wonder if you made an attempt to benchmark the http server somehow to see if there's anything we need to worry about.

I made a silly test with existing http client demo, but it's not very representative. I'll update the discussion thread with better measurement