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

Use Actix as base #35

Merged
merged 11 commits into from
Jul 15, 2021
Merged

Use Actix as base #35

merged 11 commits into from
Jul 15, 2021

Conversation

JackThomson2
Copy link
Contributor

@JackThomson2 JackThomson2 commented Jul 2, 2021

Actix is a very fast framework and is at the top of the TechEmpowered benchmarks

In my testing it is considerably faster than the hyper backend:

Hyper:

oha -n 100000 http://localhost:5000
Summary:
  Success rate:	1.0000
  Total:	29.8708 secs
  Slowest:	0.1254 secs
  Fastest:	0.0007 secs
  Average:	0.0149 secs
  Requests/sec:	3347.7546

  Total data:	1.80 MiB
  Size/request:	18 B
  Size/sec:	61.67 KiB

Response time histogram:
  0.004 [5702]  |■■■■■■
  0.008 [29544] |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  0.012 [26085] |■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  0.016 [13583] |■■■■■■■■■■■■■■
  0.020 [6547]  |■■■■■■■
  0.024 [3716]  |■■■■
  0.028 [2673]  |■■
  0.032 [2312]  |■■
  0.036 [2227]  |■■
  0.040 [1393]  |■
  0.044 [6218]  |■■■■■■

Latency distribution:
  10% in 0.0054 secs
  25% in 0.0074 secs
  50% in 0.0107 secs
  75% in 0.0167 secs
  90% in 0.0324 secs
  95% in 0.0427 secs
  99% in 0.0607 secs

Actix:

oha -n 100000 http://localhost:5000
Summary:
  Success rate:	1.0000
  Total:	10.2622 secs
  Slowest:	0.0552 secs
  Fastest:	0.0003 secs
  Average:	0.0051 secs
  Requests/sec:	9744.5392

  Total data:	1.91 MiB
  Size/request:	20 B
  Size/sec:	190.32 KiB

Response time histogram:
  0.001 [1064]  |
  0.003 [17160] |■■■■■■■■■■■■■■■
  0.004 [36476] |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  0.005 [24615] |■■■■■■■■■■■■■■■■■■■■■
  0.007 [9881]  |■■■■■■■■
  0.008 [3776]  |■■■
  0.010 [1449]  |■
  0.011 [928]   |
  0.012 [633]   |
  0.014 [510]   |
  0.015 [3508]  |■■■

Latency distribution:
  10% in 0.0026 secs
  25% in 0.0033 secs
  50% in 0.0042 secs
  75% in 0.0054 secs
  90% in 0.0073 secs
  95% in 0.0107 secs
  99% in 0.0254 secs

@sansyrox
Copy link
Member

sansyrox commented Jul 3, 2021

@JackThomson2 , is your PR ready for review or are you still working on it??

@JackThomson2
Copy link
Contributor Author

@sansyrox Yes it's ready. Key new features are: Much faster performance, ability to set custom response headers and passing the post body to post functions

@JackThomson2 JackThomson2 marked this pull request as ready for review July 3, 2021 08:47
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JackThomson2 👋

Thank you for the amazing work 🚀 .
I have some questions can you please answer them before we move ahead?

Thanks.

robyn/__init__.py Show resolved Hide resolved
src/processor.rs Show resolved Hide resolved
// pyO3 module
use actix_web::*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we switch this to individual imports instead of importing everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can do, its actix convention in the docs to import all

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JackThomson2 , please change it to import specific items instead of an import all and then we can merge this PR.

src/server.rs Show resolved Hide resolved
src/server.rs Show resolved Hide resolved
test.py Show resolved Hide resolved
test.py Show resolved Hide resolved
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, approved it by mistake 😅

self.add_route("POST", endpoint, handler)

return inner

def put(self, endpoint):
Copy link
Member

@sansyrox sansyrox Jul 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JackThomson2 , I think if we copy your POST implementation to PUT method, it should also work well imo. What do you think?

@sansyrox
Copy link
Member

sansyrox commented Jul 12, 2021

Hey @JackThomson2 👋
Did you get a chance to look at this PR?

@JackThomson2
Copy link
Contributor Author

Hi, sorry got pretty side-tracked this weekend, will need a bit of a rebase before we can merge, I'll see if I have time tomorrow

@sansyrox
Copy link
Member

Hi, sorry got pretty side-tracked this weekend, will need a bit of a rebase before we can merge, I'll see if I have time tomorrow

@JackThomson2 , yep yep no worries. I just got really excited after seeing the response time reduce to 1/3rd of itself 😅

@JackThomson2
Copy link
Contributor Author

JackThomson2 commented Jul 13, 2021

Yes it's a huge upgrade, using actix opens a lot more opportunities for file serving or websockets etc.

I'm going to look at implementing https://github.com/davidhewitt/pythonize which would allow us to serialise the json from the rust side rather than python side.

@JackThomson2
Copy link
Contributor Author

JackThomson2 commented Jul 14, 2021

@sansyrox Want to have a look now? Please test this on your machine before merging :)

@sansyrox
Copy link
Member

@JackThomson2 , this looks great ✨ . I will test it on my machine today evening and will rebase merge it then.

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well. LGTM 🚀

@sansyrox sansyrox merged commit f15daae into sparckles:main Jul 15, 2021
@sansyrox
Copy link
Member

@JackThomson2 , this was an amazing PR 🔥
I had to squash merge this instead of a rebase merge as the conflicts were too much to handle in 2 merge commits.

@sansyrox
Copy link
Member

I'm going to look at implementing https://github.com/davidhewitt/pythonize which would allow us to serialise the json from the rust side rather than python side.

@JackThomson2 , I also tried using this but since the incoming response has no fixed structure(read nested json) , I thought it wouldn't be possible to extract the object in a map.

It will be amazing it we could find a way to bypass it.

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

Successfully merging this pull request may close these issues.

2 participants