refactor(ext/http): rewrite hyper integration and fix bug #12332

piscisaureus · 2021-10-05T19:29:24Z

No description provided.

CLAassistant · 2021-10-15T05:47:06Z

All committers have signed the CLA.

This avoids a bunch of duplicated code.

lucacasonato

Took a first look at this, and it is looking very good. The code is very readable, and a lot less prone to poll-after-ready issues. Have you done benchmarks yet?

AaronO · 2021-10-29T09:49:46Z

ext/http/lib.rs

- ("op_http_response_close", op_async(op_http_response_close)),
+ ("op_http_accept", op_async(op_http_accept)),
+ ("op_http_read", op_async(op_http_read)),
+ ("op_http_write_response", op_async(op_http_write_response)),


Might make sense to call this op_http_respond instead of op_http_write_response

I think op_http_write_headers makes more sense actually.
(I know it also can write the body, but I see that as a performance hack, not a clean abstraction).

…2332) Fixes: denoland#12193

ry · 2021-11-05T19:31:47Z

ext/http/01_http.js

 method,
 headersList,
 url,
 ] = nextRequest;
+ SetPrototypeAdd(this.managedResources, streamRid);


It seems there is only one resource placed into managedResources now. Can managedResources be removed entirely?

The managedResources set is associated with an HttpConn instance. It contains multiple HttpStreamResources.

ry · 2021-11-05T19:35:23Z

runtime/ops/http.rs

@@ -29,7 +30,7 @@ fn op_http_start(
 let (read_half, write_half) = resource.into_inner();
 let tcp_stream = read_half.reunite(write_half)?;
 let addr = tcp_stream.local_addr()?;
- return deno_http::start_http(state, tcp_stream, addr, "http");
+ return http_create_conn_resource(state, tcp_stream, addr, "http");
 }


Not actionable for this PR, just an observation:

The runtime/ops/http.rs module seems useless. Why aren't these ops just in the ext/http/?

ry · 2021-11-05T19:36:39Z

runtime/web_worker.rs

@@ -587,6 +585,5 @@ pub fn run_web_worker(
 debug!("Worker thread shuts down {}", &name);
 result
 };
-
- rt.block_on(fut)
+ run_basic(fut)


unrelated changes?

Necessary, but in a separate commit.

ry · 2021-11-05T19:50:03Z

ext/http/lib.rs

+ let (request_tx, request_rx) = oneshot::channel();
+ let (response_tx, response_rx) = oneshot::channel();


I would guess the use of channels here is why the refactor is 10% slower.

I don't think so.

ry · 2021-11-05T19:51:37Z

ext/http/lib.rs

+ // A local task that polls the hyper connection future to completion.
+ let task_fut = async move {
+ pin_mut!(shutdown_fut);
+ pin_mut!(conn_fut);
+ let result = match select(conn_fut, shutdown_fut).await {
+ Either::Left((result, _)) => result,
+ Either::Right((_, mut conn_fut)) => {
+ conn_fut.as_mut().graceful_shutdown();
+ conn_fut.await
+ }
+ };
+ filter_enotconn(result).map_err(Arc::from)
+ };
+ let (task_fut, closed_fut) = task_fut.remote_handle();
+ let closed_fut = closed_fut.shared();
+ spawn_local(task_fut);


Very fancy, but apparently working.

Hopefully this organization will allow us to experiment with running the hyper server on a separate thread more easily.

Very fancy, but apparently working.

What would you suggest that's less fancy?

ext/http/lib.rs

This avoids a bunch of duplicated code.

…2332) Fixes: denoland#12193

This avoids a bunch of duplicated code.

…2332) Fixes: denoland#12193

ry

LGTM

Combining the resources is a big clean up
The rust side of the code is much easier to reason about.
This also fixes a bug

However this patch also degrades throughput performance by about 4% on hello-world server. I've done another perf analysis below. We think the benefits of this refactor outweigh the slowness. We hope this serves as good foundation for further improvements in ext/http.

Performance Analysis 2

Using: ./target/release/deno run --allow-net cli/bench/deno_http_native.js and wrk --latency -d 20s http:https://127.0.0.1:4500

Summary: throughput degraded by 4%, latency seems to be marginally better.

Deno v1.15.3

Running 20s test @ http:https://127.0.0.1:4500
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    99.53us   99.42us   7.86ms   99.30%
    Req/Sec    48.21k     1.69k   50.66k    99.00%
  Latency Distribution
     50%   94.00us
     75%  112.00us
     90%  135.00us
     99%  186.00us
  1928035 requests in 20.10s, 159.97MB read
Requests/sec:  95922.25
Transfer/sec:      7.96MB

Running 20s test @ http:https://127.0.0.1:4500
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    98.50us   37.24us   3.58ms   82.77%
    Req/Sec    47.92k     1.61k   49.61k    99.00%
  Latency Distribution
     50%   95.00us
     75%  113.00us
     90%  136.00us
     99%  187.00us
  1916882 requests in 20.10s, 159.04MB read
Requests/sec:  95370.21
Transfer/sec:      7.91MB

This Patch

(version bb64927)

Running 20s test @ http:https://127.0.0.1:4500
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   101.77us   32.60us   3.81ms   83.89%
    Req/Sec    46.00k     1.40k   47.69k    98.01%
  Latency Distribution
     50%  100.00us
     75%  115.00us
     90%  131.00us
     99%  167.00us
  1840014 requests in 20.10s, 152.67MB read
Requests/sec:  91543.34
Transfer/sec:      7.60MB

Running 20s test @ http:https://127.0.0.1:4500
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   101.91us   29.00us   3.19ms   79.30%
    Req/Sec    45.90k     1.40k   47.43k    98.26%
  Latency Distribution
     50%  101.00us
     75%  115.00us
     90%  132.00us
     99%  167.00us
  1835615 requests in 20.10s, 152.30MB read
Requests/sec:  91323.73
Transfer/sec:      7.58MB

Node v17.0.1 baseline

Running 20s test @ http:https://127.0.0.1:4544
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   100.67us  171.98us  12.41ms   99.67%
    Req/Sec    50.30k     2.86k   53.15k    98.01%
  Latency Distribution
     50%   89.00us
     75%  115.00us
     90%  150.00us
     99%  209.00us
  2011687 requests in 20.10s, 257.08MB read
Requests/sec: 100084.42
Transfer/sec:     12.79MB

Running 20s test @ http:https://127.0.0.1:4544
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    98.57us  114.68us   9.48ms   99.17%
    Req/Sec    50.33k     2.81k   52.86k    97.76%
  Latency Distribution
     50%   89.00us
     75%  115.00us
     90%  150.00us
     99%  208.00us
  2012718 requests in 20.10s, 257.21MB read
Requests/sec: 100134.06
Transfer/sec:     12.80MB

This avoids a bunch of duplicated code.

…2332) Fixes: denoland#12193

…noland#12332)" This reverts commit 5b1e537.

…2332)" (#12704) This reverts commit 5b1e537.

…2732) Fixes: denoland#12193 Fixes: denoland#12251 Closes: denoland#12714

piscisaureus force-pushed the http branch 3 times, most recently from 9c01b43 to 71b312a Compare October 9, 2021 06:26

piscisaureus linked an issue Oct 12, 2021 that may be closed by this pull request

Multiple overlapping HttpConn#nextRequest() promises are lost in space #12193

Closed

piscisaureus force-pushed the http branch 6 times, most recently from 89be132 to 9d94ef2 Compare October 20, 2021 01:01

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Oct 21, 2021

refactor: move mod tokio_util to runtime (denoland#12332)

fd2a375

This avoids a bunch of duplicated code.

piscisaureus force-pushed the http branch 3 times, most recently from a2dad15 to 8589f97 Compare October 22, 2021 01:18

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Oct 23, 2021

refactor: move mod tokio_util to runtime (denoland#12332)

2bdc292

This avoids a bunch of duplicated code.

piscisaureus force-pushed the http branch 8 times, most recently from 64dbbae to 343ba0d Compare October 24, 2021 18:17

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Oct 26, 2021

refactor: move mod tokio_util to runtime (denoland#12332)

44b46c4

This avoids a bunch of duplicated code.

piscisaureus force-pushed the http branch from 343ba0d to d41b5e1 Compare October 26, 2021 21:46

lucacasonato reviewed Oct 28, 2021

View reviewed changes

piscisaureus requested review from ry and AaronO October 28, 2021 19:03

AaronO reviewed Oct 29, 2021

View reviewed changes

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 4, 2021

refactor(ext/http): rewrite hyper integration and fix bug (denoland#1…

9d04d14

…2332) Fixes: denoland#12193

piscisaureus force-pushed the http branch from c586f07 to 9d04d14 Compare November 4, 2021 14:41

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 4, 2021

refactor(ext/http): rewrite hyper integration and fix bug (denoland#1…

0213cdc

…2332) Fixes: denoland#12193

piscisaureus force-pushed the http branch from 9d04d14 to 0213cdc Compare November 4, 2021 14:43

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 4, 2021

perf: optimize some important crates more aggressively (denoland#12332)

90586cb

ry reviewed Nov 5, 2021

View reviewed changes

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 5, 2021

perf(core): optimize waker capture in AsyncRefCell (denoland#12332)

bc4b7da

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 5, 2021

perf: optimize some important crates more aggressively (denoland#12332)

a6ed1b6

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 5, 2021

refactor: move mod tokio_util to runtime (denoland#12332)

a54b293

This avoids a bunch of duplicated code.

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 5, 2021

refactor(ext/http): rewrite hyper integration and fix bug (denoland#1…

1f8e718

…2332) Fixes: denoland#12193

piscisaureus force-pushed the http branch from 90586cb to 38aa44d Compare November 5, 2021 21:33

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 5, 2021

perf(core): optimize waker capture in AsyncRefCell (denoland#12332)

8b2fb67

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 5, 2021

perf: optimize some important crates more aggressively (denoland#12332)

ffcf891

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 5, 2021

refactor: move mod tokio_util to runtime (denoland#12332)

a39cfe6

This avoids a bunch of duplicated code.

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Nov 5, 2021

refactor(ext/http): rewrite hyper integration and fix bug (denoland#1…

bb64927

…2332) Fixes: denoland#12193

piscisaureus force-pushed the http branch from 38aa44d to bb64927 Compare November 5, 2021 21:35

ry approved these changes Nov 5, 2021

View reviewed changes

piscisaureus added 4 commits November 8, 2021 12:49

perf(core): optimize waker capture in AsyncRefCell (denoland#12332)

4c46862

perf: optimize some important crates more aggressively (denoland#12332)

b042697

refactor: move mod tokio_util to runtime (denoland#12332)

f1b1a3f

This avoids a bunch of duplicated code.

refactor(ext/http): rewrite hyper integration and fix bug (denoland#1…

5b1e537

…2332) Fixes: denoland#12193

piscisaureus force-pushed the http branch from bb64927 to 5b1e537 Compare November 8, 2021 20:49

piscisaureus merged commit 5b1e537 into denoland:main Nov 8, 2021

piscisaureus deleted the http branch November 8, 2021 21:43

kt3k mentioned this pull request Nov 9, 2021

chore: skip 'serve should handle websocket requests' denoland/std#1537

Closed

lucacasonato added a commit to lucacasonato/deno that referenced this pull request Nov 9, 2021

Revert "refactor(ext/http): rewrite hyper integration and fix bug (de…

c5c319f

…noland#12332)" This reverts commit 5b1e537.

lucacasonato mentioned this pull request Nov 9, 2021

Multiple overlapping HttpConn#nextRequest() promises are lost in space #12193

Closed

lucacasonato added a commit that referenced this pull request Nov 9, 2021

Revert "refactor(ext/http): rewrite hyper integration and fix bug (#1…

75793ba

…2332)" (#12704) This reverts commit 5b1e537.

piscisaureus referenced this pull request in piscisaureus/deno Nov 10, 2021

refactor(ext/http): rewrite hyper integration and fix bug (denoland#1…

72a6231

…2732) Fixes: denoland#12193 Fixes: denoland#12251 Closes: denoland#12714

piscisaureus mentioned this pull request Nov 10, 2021

reland #12332: refactor(ext/http): rewrite hyper integration and fix bug #12732

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ext/http): rewrite hyper integration and fix bug #12332

refactor(ext/http): rewrite hyper integration and fix bug #12332

piscisaureus commented Oct 5, 2021 •

edited

Loading

CLAassistant commented Oct 15, 2021 •

edited

Loading

lucacasonato left a comment

AaronO Oct 29, 2021

piscisaureus Oct 29, 2021

ry Nov 5, 2021

piscisaureus Nov 5, 2021

ry Nov 5, 2021

ry Nov 5, 2021

piscisaureus Nov 5, 2021

ry Nov 5, 2021

piscisaureus Nov 5, 2021

ry Nov 5, 2021

piscisaureus Nov 5, 2021

ry left a comment •

edited

Loading

		let (request_tx, request_rx) = oneshot::channel();
		let (response_tx, response_rx) = oneshot::channel();

refactor(ext/http): rewrite hyper integration and fix bug #12332

refactor(ext/http): rewrite hyper integration and fix bug #12332

Conversation

piscisaureus commented Oct 5, 2021 • edited Loading

CLAassistant commented Oct 15, 2021 • edited Loading

lucacasonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment • edited Loading

Choose a reason for hiding this comment

Performance Analysis 2

Deno v1.15.3

This Patch

Node v17.0.1 baseline

piscisaureus commented Oct 5, 2021 •

edited

Loading

CLAassistant commented Oct 15, 2021 •

edited

Loading

ry left a comment •

edited

Loading