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

ASGI Support #532

Open
wants to merge 91 commits into
base: master
from

Conversation

@masipcat
Copy link
Contributor

masipcat commented Jun 15, 2019

dmanchon and others added 14 commits Jun 13, 2019
@masipcat masipcat requested review from bloodbare and vangheem Jun 15, 2019
@masipcat masipcat referenced this pull request Jun 15, 2019
masipcat and others added 3 commits Jun 15, 2019
@dmanchon

This comment has been minimized.

Copy link
Member

dmanchon commented Jun 15, 2019

TODO:

  • get rid of starlette.websockets.WebSocket dependency
  • move WebSocketSession to async_asgi_testclient
  • improve the IRequest interface
  • AsgiStreamReader is not able to support HTTP2 should be reimplemented correctly
dmanchon and others added 7 commits Jun 15, 2019
@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Sep 24, 2019

Awesome! If you have time, can you compare that to Guillotina 4/5?

@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Sep 24, 2019

I ran the benchmark again using more process/workers and with a duration of 60s:

Command: molotov molotov/test.py -p 8 -w 30 -d 60 -x --uvloop

Db disk          
  Measure 1 Measure 2   Avg req/s Diff
G4+uvloop 8497 8716   143,44 0,00%
G5+uvloop 7090 6468   112,98 -21,23%
G6 Uvicorn 0.9.0 8299 8162   137,18 -4,37%
G6 Hypercorn+uvloop 0.8.2 8150 8161   135,93 -5,24%
Db in memory          
  Measure 1 Measure 2   Avg req/s Diff
G4+uvloop 33364 33121   554,04 0,00%
G5+uvloop 30147 30462   505,08 -8,84%
G6 Uvicorn 0.9.0 32291 33048   544,49 -1,72%
G6 Hypercorn+uvloop 0.8.2 28297 28098 469,96 -15,18%

I think that the benchmark "Db in memory" is the most adequate to compare the performance between asgi vs aiohttp, but doesn't show the performance in a production environment (using postgres).

@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Sep 24, 2019

image

I've been playing with pyflames this weekend and I think that we could improve the performance avoiding yarl.URL. Right now we are only using request.url in:

url = request.url.human_repr()

url = req.url.with_path(path)

The asgi server already provides the parts of the url splitted (scheme, method, headers, raw_path, query_string) and the guillotina request converts the query_string and headers to multidict, therefore the only thing is missing is providing the full url (but should be as easy as concatenate all components).

What do you think about removing yarl @vangheem @bloodbare ?

Here is the svg if you want to check yourself: out.txt (rename txt to svg)

@bloodbare

This comment has been minimized.

Copy link
Member

bloodbare commented Sep 24, 2019

I like the concept of removing yarl, composing URLs on the get_url is used a lot, which is important to push performance there. We need to be able to provide our own path and switch the schema, do you think that manual replacement will be faster than yarl ?

@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Sep 24, 2019

We need to be able to provide our own path and switch the schema, do you think that manual replacement will be faster than yarl ?

Well, right now we are constructing the yarl.URL twice. First time when we are calling request.url and second time in get_url() with req.url.with_path(path). I'll try doing the concatenation with https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlunparse just one time and see if it's faster. I hope it will be faster :P I'll publish the results when it's done!

@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Sep 24, 2019

Updated benchmark after removing yarl:

Db disk        
  Measure 1 Measure 2 Avg req/s Diff
G4+uvloop 8497 8716 143,44 0,00%
G5+uvloop 7090 6468 112,98 -21,23%
G6 Uvicorn 0.9.0 8299 8162 137,18 -4,37%
G6 Uvicorn 0.9.0 (without yarl) 8759 8610 144,74 0,91%
G6 Hypercorn+uvloop 0.8.2 8150 8161 135,93 -5,24%
Db in memory        
  Measure 1 Measure 2 Avg req/s Diff
G4+uvloop 33364 33121 554,04 0,00%
G5+uvloop 30147 30462 505,08 -8,84%
G6 Uvicorn 0.9.0 32291 33048 544,49 -1,72%
G6 Uvicorn 0.9.0 (without yarl) 35548 35009 582,68 5,17%
G6 Hypercorn+uvloop 0.8.2 28297 28098 469,96 -15,18%
@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Sep 24, 2019

Awesome job!

masipcat added 2 commits Oct 2, 2019
@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Oct 2, 2019

I'm ready for a review @bloodbare @vangheem ! and I think this is ready as a first alpha

@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Oct 2, 2019

thank you! I will review tonight.

Just to be clear and make sure everyone is on the same page:

  • cut 5.x branch
  • merge this to master
  • cut alpha release

@bloodbare @jordic We ready to for this?

masipcat added 4 commits Oct 2, 2019
@masipcat masipcat force-pushed the asgi-support branch from 4a2020b to 515327a Oct 18, 2019
zope.interface==4.4.3
# Force build uvloop from source instead of using wheel
# Replace with a uvicorn version that uses uvloop built for python 3.8
git+https://github.com/dmontagu/uvloop@patch-1

This comment has been minimized.

Copy link
@masipcat

masipcat Oct 18, 2019

Author Contributor

^ Temporary fix. I guess this would be fixed in the following days.

This comment has been minimized.

Copy link
@bloodbare

bloodbare Oct 18, 2019

Member

That may delay the merge

@bloodbare

This comment has been minimized.

Copy link
Member

bloodbare commented Oct 18, 2019

@masipcat that holds the merge until uvloops merge is done?

@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Oct 18, 2019

@bloodbare The problem with uvloop is that the official release is built with Cython <0.29.12 (doesn't support python 3.8) and is distributed with binaries (python wheels). Building uvloop from source using Cython 0.29.13 works without any other change.

So, I don't think we need to wait, but it's up to you ;)

docs/source/training/extending/websockets.md Outdated Show resolved Hide resolved
guillotina/asgi.py Show resolved Hide resolved
zope.interface==4.4.3
# Force build uvloop from source instead of using wheel
# Replace with a uvicorn version that uses uvloop built for python 3.8
git+https://github.com/dmontagu/uvloop@patch-1

This comment has been minimized.

Copy link
@bloodbare

bloodbare Oct 18, 2019

Member

That may delay the merge

masipcat added 2 commits Oct 18, 2019
@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Oct 23, 2019

Thoughts on git+https://github.com/dmontagu/uvloop@patch-1? Maybe not depend on 3.8 yet?

@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Oct 23, 2019

I removed python 3.8 on travis and removed the git+https... in the requirements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.