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

Perf improv #2074

Merged
merged 4 commits into from
Mar 21, 2021
Merged

Perf improv #2074

merged 4 commits into from
Mar 21, 2021

Conversation

ahopkins
Copy link
Member

@ahopkins ahopkins commented Mar 21, 2021

I was playing around with some benchmarking and started to notice that we were not realizing the performance bump that I was expecting.

v20.12

Running 10s test @ http://localhost:3000
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.60ms    3.62ms  77.03ms   92.43%
    Req/Sec    10.73k     2.71k   26.83k    79.17%
  1281439 requests in 10.05s, 144.20MB read
Requests/sec: 127460.90
Transfer/sec:     14.34MB

master

Running 10s test @ http://localhost:3000
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.63ms    4.25ms  87.71ms   92.34%
    Req/Sec    11.33k     2.63k   52.29k    84.13%
  1343733 requests in 10.08s, 131.99MB read
Requests/sec: 133339.69
Transfer/sec:     13.10MB

I realized that this is mainly due to some minor issues in handle_request. With these small changes:

perf-improv

Running 10s test @ http://localhost:3000
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.58ms    4.12ms  88.78ms   92.44%
    Req/Sec    11.49k     2.40k   50.72k    77.39%
  1376168 requests in 10.10s, 135.18MB read
Requests/sec: 136254.12
Transfer/sec:     13.38MB

There are some more gains to be had here, but not until 21.6 and a more thorough overhaul of this method.

@ahopkins ahopkins requested a review from a team as a code owner March 21, 2021 00:44
@codecov
Copy link

codecov bot commented Mar 21, 2021

Codecov Report

Merging #2074 (f9bb218) into master (8a2ea62) will decrease coverage by 0.018%.
The diff coverage is 100.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##            master     #2074       +/-   ##
=============================================
- Coverage   92.217%   92.200%   -0.018%     
=============================================
  Files           38        38               
  Lines         3482      3487        +5     
  Branches       583       584        +1     
=============================================
+ Hits          3211      3215        +4     
  Misses         184       184               
- Partials        87        88        +1     
Impacted Files Coverage Δ
sanic/app.py 92.213% <100.000%> (-0.063%) ⬇️
sanic/request.py 97.712% <100.000%> (+0.085%) ⬆️
sanic/router.py 94.521% <100.000%> (-0.146%) ⬇️
sanic/http.py 77.491% <0.000%> (-0.369%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a2ea62...f9bb218. Read the comment docs.

Copy link
Member

@ashleysommer ashleysommer left a comment

Choose a reason for hiding this comment

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

Looks like a good change, makes the code easier to follow and is a bit faster.

I still want to look further into some performance comparisons, there's some other things I want to check out (eg, last time I tested the streaming server changes, the performance was exactly the same between uvloop and plain asyncio).

@ahopkins ahopkins merged commit 15a8b5c into master Mar 21, 2021
@ahopkins ahopkins deleted the perf-improv branch March 21, 2021 07:47
@ahopkins
Copy link
Member Author

Agreed. I think a large focus for the next three months is to scrutinize every line that is executed in the request/response cycle.

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