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

Remove dispatch optimization #2732

Merged
merged 3 commits into from
Aug 6, 2019
Merged

Remove dispatch optimization #2732

merged 3 commits into from
Aug 6, 2019

Conversation

ry
Copy link
Member

@ry ry commented Aug 5, 2019

Deno.core.dispatch() used to push the "control" buf onto the shared
array buffer before calling into V8, with the idea that it was one less
argument to parse. Turns out there is no more overhead passing the
control ArrayBuffer directly over. Furthermore this optimization was
making the refactors outlined in #2730 more complex. Therefore it is
being removed.

benchmark results are unchanged:

this branch

~/src/deno> wrk -d 30s --latency http://127.0.0.1:4500
Running 30s test @ http://127.0.0.1:4500
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   392.64us    2.76ms  88.67ms   98.88%
    Req/Sec    21.34k     1.56k   24.37k    86.50%
  Latency Distribution
     50%  197.00us
     75%  242.00us
     90%  310.00us
     99%    3.38ms
  1274182 requests in 30.01s, 61.97MB read
Requests/sec:  42457.87
Transfer/sec:      2.07MB

deno v0.13

~/src/deno> wrk -d 30s --latency http://127.0.0.1:4500
Running 30s test @ http://127.0.0.1:4500
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   269.44us  427.67us   7.88ms   97.32%
    Req/Sec    21.17k     1.12k   24.37k    78.17%
  Latency Distribution
     50%  199.00us
     75%  248.00us
     90%  315.00us
     99%    2.93ms
  1264226 requests in 30.01s, 61.49MB read
Requests/sec:  42127.59
Transfer/sec:      2.05MB

ry added 2 commits August 5, 2019 19:12
Deno.core.dispatch() used to push the "control" buf onto the shared
array buffer before calling into V8, with the idea that it was one less
argument to parse. Turns out there is no more overhead passing the
control ArrayBuffer directly over. Furthermore this optimization was
making the refactors outlined in denoland#2730 more complex. Therefore it is
being removed.
@@ -871,43 +855,6 @@ pub mod tests {
});
}

#[test]
fn test_shared() {
Copy link
Member Author

@ry ry Aug 5, 2019

Choose a reason for hiding this comment

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

This test was testing exactly the optimization that was removed.

@ry ry requested a review from piscisaureus August 5, 2019 23:45
@ry
Copy link
Member Author

ry commented Aug 5, 2019

travis benchmarks for this branch:

{
  "req_per_sec": {
    "deno_tcp": 52181, 
    "deno_core_single": 54048, 
    "node_proxy_tcp": 20092, 
    "deno_proxy_tcp": 18167, 
    "deno_core_multi": 56638, 
    "deno_tcp_current_thread": 51249, 
    "hyper": 59218, 
    "node_tcp": 57930, 
    "deno_http": 19178, 
    "node_http": 24727, 
    "deno_proxy": 1663, 
    "node_proxy": 2740
  }, 
  "max_latency": {
    "deno_tcp": 8.82, 
    "deno_core_single": 0.496, 
    "node_proxy_tcp": 1.1, 
    "deno_proxy_tcp": 5.56, 
    "deno_core_multi": 1.3, 
    "deno_tcp_current_thread": 9.09, 
    "hyper": 0.846, 
    "node_tcp": 0.818, 
    "deno_http": 2.64, 
    "node_http": 1.96, 
    "deno_proxy": 30.85, 
    "node_proxy": 9.46
  }, 

For master branch

{
  "req_per_sec": {
    "deno_tcp": 51184, 
    "deno_core_single": 57055, 
    "node_proxy_tcp": 19587, 
    "deno_proxy_tcp": 17110, 
    "deno_core_multi": 51390, 
    "deno_tcp_current_thread": 48308, 
    "hyper": 61656, 
    "node_tcp": 56250, 
    "deno_http": 18386, 
    "node_http": 24297, 
    "deno_proxy": 1532, 
    "node_proxy": 2607
  }, 
  "max_latency": {
    "deno_tcp": 8.93, 
    "deno_core_single": 0.451, 
    "node_proxy_tcp": 1.2, 
    "deno_proxy_tcp": 6.0, 
    "deno_core_multi": 1.37, 
    "deno_tcp_current_thread": 9.57, 
    "hyper": 1.11, 
    "node_tcp": 0.684, 
    "deno_http": 2.8, 
    "node_http": 2.52, 
    "deno_proxy": 48.5, 
    "node_proxy": 10.81
  }, 

@ry ry merged commit 046cccf into denoland:master Aug 6, 2019
@ry ry deleted the remove_optimization branch August 6, 2019 00:12
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