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

feat: Minimal op registration in isolate #3002

Merged
merged 35 commits into from
Sep 30, 2019

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Sep 20, 2019

Towards #2987

This PR introduces OpRegistry structure in Isolate.

OpRegistry collects OpHandlers which are functions returning CoreOp.

This can be safely introduced to work with deno_cli because if you set dispatch manually via set_dispatch then ops registered on isolate are ignored.

I still have to get op id forwarding to JS.

CC @ry @piscisaureus

core/isolate.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

$ cargo run --release --example deno_core_http_bench

third_party/wrk/mac/wrk -d 10s --latency http://127.0.0.1:4544
Running 10s test @ http://127.0.0.1:4544
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   121.98us   41.94us   2.00ms   93.58%
    Req/Sec    39.43k     2.08k   41.72k    82.18%
  Latency Distribution
     50%  114.00us
     75%  119.00us
     90%  142.00us
     99%  292.00us
  792454 requests in 10.10s, 38.54MB read
Requests/sec:  78458.19
Transfer/sec:      3.82MB
$  cargo run --release --example deno_core_http_bench -- --multi-thread

third_party/wrk/mac/wrk -d 10s --latency http://127.0.0.1:4544
Running 10s test @ http://127.0.0.1:4544
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   185.81us  613.64us  18.64ms   98.18%
    Req/Sec    36.48k     6.32k   43.68k    87.13%
  Latency Distribution
     50%  106.00us
     75%  142.00us
     90%  228.00us
     99%    1.22ms
  733098 requests in 10.10s, 35.66MB read
Requests/sec:  72578.50
Transfer/sec:      3.53MB

core/examples/http_bench.js Outdated Show resolved Hide resolved
core/ops.rs Outdated Show resolved Hide resolved
core/shared_queue.js Outdated Show resolved Hide resolved
@kevinkassimo
Copy link
Contributor

$ cargo run --release --example deno_core_http_bench

third_party/wrk/mac/wrk -d 10s --latency http://127.0.0.1:4544
Running 10s test @ http://127.0.0.1:4544
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   121.98us   41.94us   2.00ms   93.58%
    Req/Sec    39.43k     2.08k   41.72k    82.18%
  Latency Distribution
     50%  114.00us
     75%  119.00us
     90%  142.00us
     99%  292.00us
  792454 requests in 10.10s, 38.54MB read
Requests/sec:  78458.19
Transfer/sec:      3.82MB
$  cargo run --release --example deno_core_http_bench -- --multi-thread

third_party/wrk/mac/wrk -d 10s --latency http://127.0.0.1:4544
Running 10s test @ http://127.0.0.1:4544
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   185.81us  613.64us  18.64ms   98.18%
    Req/Sec    36.48k     6.32k   43.68k    87.13%
  Latency Distribution
     50%  106.00us
     75%  142.00us
     90%  228.00us
     99%    1.22ms
  733098 requests in 10.10s, 35.66MB read
Requests/sec:  72578.50
Transfer/sec:      3.53MB

Do you have the comparison with the master branch?

@bartlomieju
Copy link
Member Author

@kevinkassimo

master

"req_per_sec": {
    "deno_tcp": 53518, 
    "deno_core_single": 54066, 
    "node_proxy_tcp": 20393, 
    "deno_proxy_tcp": 18601, 
    "deno_core_multi": 58185, 
    "deno_tcp_current_thread": 51059, 
    "hyper": 63312, 
    "node_tcp": 54550, 
    "deno_http": 18684, 
    "node_http": 19992, 
    "deno_proxy": 990, 
    "node_proxy": 2640
  }, 
  "max_latency": {
    "deno_tcp": 9.04, 
    "deno_core_single": 0.36, 
    "node_proxy_tcp": 1.05, 
    "deno_proxy_tcp": 5.36, 
    "deno_core_multi": 0.95, 
    "deno_tcp_current_thread": 9.46, 
    "hyper": 2.86, 
    "node_tcp": 0.597, 
    "deno_http": 2.62, 
    "node_http": 2.22, 
    "deno_proxy": 19.51, 
    "node_proxy": 9.4
  }, 

this PR

"req_per_sec": {
    "deno_tcp": 51090, 
    "deno_core_single": 58520, 
    "node_proxy_tcp": 19952, 
    "deno_proxy_tcp": 18476, 
    "deno_core_multi": 61798, 
    "deno_tcp_current_thread": 48471, 
    "hyper": 63395, 
    "node_tcp": 53003, 
    "deno_http": 18526, 
    "node_http": 19925, 
    "deno_proxy": 961, 
    "node_proxy": 2533
  }, 
  "max_latency": {
    "deno_tcp": 8.89, 
    "deno_core_single": 0.405, 
    "node_proxy_tcp": 1.06, 
    "deno_proxy_tcp": 5.62, 
    "deno_core_multi": 1.07, 
    "deno_tcp_current_thread": 9.13, 
    "hyper": 2.56, 
    "node_tcp": 0.588, 
    "deno_http": 2.72, 
    "node_http": 2.28, 
    "deno_proxy": 18.11, 
    "node_proxy": 10.16
  }, 

That's a nice speed up in req/secs, but latency is slightly worse.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Looks good! This is what I had in mind :)

core/ops.rs Outdated Show resolved Hide resolved
core/shared_queue.js Outdated Show resolved Hide resolved
core/isolate.rs Outdated Show resolved Hide resolved
core/isolate.rs Show resolved Hide resolved
core/isolate.rs Outdated Show resolved Hide resolved
core/ops.rs Show resolved Hide resolved
core/isolate.rs Show resolved Hide resolved
core/isolate.rs Outdated Show resolved Hide resolved
core/ops.rs Outdated Show resolved Hide resolved
core/ops.rs Outdated Show resolved Hide resolved
@ry
Copy link
Member

ry commented Sep 26, 2019

Bartek - proxies shouldn’t be involved - they may kick us out of optimizations. I guess that Deno.ops[“foo”] should be an explicit function call instead. Maybe Deno.ops.get(“foo”)?

@bartlomieju
Copy link
Member Author

bartlomieju commented Sep 26, 2019

Bartek - proxies shouldn’t be involved - they may kick us out of optimizations. I guess that Deno.ops[“foo”] should be an explicit function call instead. Maybe Deno.ops.get(“foo”)?

Aaaah I was worried that's not the best solution. Yes, Deno.ops.get("foo") is the way to go, updating.

EDIT: Although I'm not quite convinced it should be Deno.ops instead of Deno.core.ops. Why introduce another global namespace?

core/isolate.rs Outdated Show resolved Hide resolved
core/ops.rs Outdated Show resolved Hide resolved
core/shared_queue.js Outdated Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM
Nice work! Let’s watch to see how this effects the benchmarks of deno_core_single and deno_core_multi

@ry ry merged commit ffbf0c2 into denoland:master Sep 30, 2019
@bartlomieju bartlomieju deleted the feat-register_op branch October 1, 2019 09:15
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.

4 participants