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

performance comparison out of date and not representative #770

Closed
samuelcolvin opened this issue Jun 3, 2017 · 10 comments
Closed

performance comparison out of date and not representative #770

samuelcolvin opened this issue Jun 3, 2017 · 10 comments

Comments

@samuelcolvin
Copy link

samuelcolvin commented Jun 3, 2017

First of all this might sound like I have beef. I don't, I'm completely beef free.

Sanic looks great, I'm a fan of its simplicity and the performance looks awesome.

That said, I think the performance comparison in your readme could do with updating. It would be great if it was:

  1. Updated to use the latest version of each framework and stated which version it's using or when it was done.
  2. Added a link to the code used.
  3. (most importantly) extended the tests slightly to add some kind of io on the server side. This is where asynchronous frameworks shine, it's also the most common case by a long way †.

I'm interested primarily in the choice between sanic and aiohttp so I did a small comparison of the two frameworks, see here. Summarising results:

  • with the server returning simple JSON with no db connection sanic is roughly 3x faster than aiohttp
  • with a single simple postgres SELECT sanic is roughly 1.6 x faster than aiohttp

This speed difference is significant but is much smaller than the performance comparison on in your readme indicates.

wildly creating numbers from thin air: 99% of requests which don't make a database, cache or http request are static and should be served with nginx etc., 99% of requests which require the sever to do any "thinking" and therefore require a web framework make tcp/udp connections server side. So measuring performance using "simple" json or text response without any kind of db/cache connection is irrelevent. I see this the whole time in performance comparisons because it's the easiest case to setup and also think "that's like choosing a car by seeing how well it works as a fridge".

@samuelcolvin
Copy link
Author

More complete results with japronto added to the mix: https://github.com/samuelcolvin/aiohttp-vs-sanic-vs-japronto

ccing the developers of aiohttp and japronto as they might have some input @asvetlov @fafhrd91 @squeaky-pl

@fafhrd91
Copy link

fafhrd91 commented Jun 3, 2017

Does performance matter if you can kill sanic server with memory overflow without any effort?

@stopspazzing
Copy link
Contributor

@samuelcolvin Could you post your system specs which you used to generate those numbers? Also have you tried many different variables, aka diff # of workers and such?

@danigosa
Copy link

danigosa commented Jun 4, 2017

@fafhrd91 could you extend your answer with some concrete examples? we are evaluating to move some parts of our current tornado API for heavy concurrent endpoints to sanic or newest aiohttp (or even golang), but as usual stability beats performance 👍

@samuelcolvin
Copy link
Author

@stopspazzing

Kernel Version: 4.10.0-21-generic
Operating System: Ubuntu 17.04
OSType: linux
Architecture: x86_64
CPUs: 8
Total Memory: 15.63GiB
Python 3.6.1
PostgreSQL 9.6.3 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 6.3.0-12ubuntu2) 6.3.0 20170406, 64-bit
all python packages: latest from pypi as of 2017-06-03

I was just running the tests locally - localhost to localhost.

I did play with the number of works, it obviously made a big difference but at present aiohttp doesn't implement multiple works natively (it currently relies on either gunicorn or ngninx) so it didn't make sense to vary that in this test.

@fafhrd91
Copy link

fafhrd91 commented Jun 5, 2017

@danigosa check this code https://github.com/channelcat/sanic/blob/master/sanic/server.py#L189
and https://github.com/channelcat/sanic/blob/master/sanic/server.py#L196

there are two paths:

  1. is_request_stream is False which is default, all benchmarks measure this code path. you can notice sanic just load everything into memory unconditionally, even without routing. So you can just start upload data to any url. application developer does not have any chance to check if request is valid or authenticated.
  2. is_request_stream is True. in that case sanic just use asyncio.Queue, and in that case application developer needs to deal with incoming stream, deal with all boilerplate code that needs to be build around raw stream. but in that case, it is easy to flood server memory with junk data.

it doesn't matter how you handle incoming data, sanic does not provide any flow control (back pressure)
so application developer needs to build it to make sanic usable in production environment. and why would you want to build it yourself when it definitely framework task?
it is interesting how many developers mistake simple with primitive.

just a note, I have never seen bottleneck in framework implementation. add logging and some business logic and performance impact from all 3 framework would be neglectable. And correctness and maintainability becomes much more important.

@asvetlov
Copy link
Contributor

asvetlov commented Jun 5, 2017

@samuelcolvin BTW why your benchmarks disable logging for sanic but keep it enabled for aiohttp?

@samuelcolvin
Copy link
Author

The set up had no log output for any of the frameworks.

I only discovered after these tests that you need to set access_log=None, logger=None to completely disable logging, I think this causes a roughly 10% speedup.

@danigosa
Copy link

danigosa commented Jun 6, 2017

@fafhrd91 Thanks for the feedback, pretty clear now :)

@r0fls
Copy link
Contributor

r0fls commented Jul 14, 2017

Since we removed the benchmark table I'm closing this issue. There is some continued discussion here for the interested: #835

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants