feat(ext/web): resourceForReadableStream #20180
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mmastrac
changed the title
mmastrac
commented
Aug 16, 2023
|
|
||
| type SenderCell = RefCell<Option<Sender<Result<BufView, Error>>>>; | ||
|
|
||
| // This indirection allows us to more easily integrate the fast streams work at a later date |
There was a problem hiding this comment.
A large amount of this code will migrate to deno_core once #20080 fully lands.
mmastrac
force-pushed
the
resource_stream
branch
from
0877136
to
4409297
Compare
mmastrac
commented
Aug 16, 2023
| @@ -141,8 141,7 @@ Deno.test("[node/http] chunked response", async () => { | |||
| } | |||
| }); | |||
|
|
|||
| // TODO(kt3k): This test case exercises the workaround for https://github.com/denoland/deno/issues/17194 | |||
| // This should be removed when #17194 is resolved. | |||
| // Test empty chunks: https://github.com/denoland/deno/issues/17194 | |||
There was a problem hiding this comment.
This test caught a legit bug, so I am removing the TODO as it's worth keeping.
mmastrac
commented
Aug 16, 2023
| await listeningPromise; | ||
| const resp = await fetch(`http://127.0.0.1:${servePort}/`); | ||
| const text = await resp.text(); | ||
| try { |
There was a problem hiding this comment.
HTTP behaves more like you would expect now -- it will always return EOF if there is an error in the middle of a stream (matching the behaviour if there was an error in any other resource).
bartlomieju
reviewed
Aug 17, 2023
| return stream; | ||
| } | ||
|
|
||
| async function asyncResponse(responseBodies, req, status, stream) { |
There was a problem hiding this comment.
It does quite a bit. This probably opens up optimization opportunities for V8.
I think it will simplify the fetch code quite a bit as well.
|
|
||
| #[op2(fast)] | ||
| #[smi] | ||
| pub fn op_readable_stream_resource_close(sender: *const c_void) { |
There was a problem hiding this comment.
How is it different that a regular close? Could it be done in Resource::close instead?
There was a problem hiding this comment.
As there are two ends to the internal "pipe", we need to be able to close them both independently. The close method on the resource propagates to this inner side through the completion handle.
Effectively this allows both sides to have independent lifetimes (though shutting down one side will shut the other down indirectly).
littledivy
pushed a commit
to littledivy/deno
that referenced
this pull request
Aug 21, 2023
Extracted from fast streams work.
This is a resource wrapper for `ReadableStream`, allowing us to treat
all `ReadableStream` instances as resources, and remove special paths in
both `fetch` and `serve`.
Performance with a ReadableStream response yields ~18% improvement:
```
return new Response(new ReadableStream({
start(controller) {
controller.enqueue(new Uint8Array([104, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100]));
controller.close();
}
})
```
This patch:
```
12:36 $ third_party/prebuilt/mac/wrk http://localhost:8080
Running 10s test @ http://localhost:8080
2 threads and 10 connections
Thread Stats Avg Stdev Max /- Stdev
Latency 99.96us 100.03us 6.65ms 98.84%
Req/Sec 47.73k 2.43k 51.02k 89.11%
959308 requests in 10.10s, 117.10MB read
Requests/sec: 94978.71
Transfer/sec: 11.59MB
```
main:
```
Running 10s test @ http://localhost:8080
2 threads and 10 connections
Thread Stats Avg Stdev Max /- Stdev
Latency 163.03us 685.51us 19.73ms 99.27%
Req/Sec 39.50k 3.98k 66.11k 95.52%
789582 requests in 10.10s, 82.83MB read
Requests/sec: 78182.65
Transfer/sec: 8.20MB
```
littledivy
pushed a commit
that referenced
this pull request
Aug 21, 2023
Extracted from fast streams work.
This is a resource wrapper for `ReadableStream`, allowing us to treat
all `ReadableStream` instances as resources, and remove special paths in
both `fetch` and `serve`.
Performance with a ReadableStream response yields ~18% improvement:
```
return new Response(new ReadableStream({
start(controller) {
controller.enqueue(new Uint8Array([104, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100]));
controller.close();
}
})
```
This patch:
```
12:36 $ third_party/prebuilt/mac/wrk http://localhost:8080
Running 10s test @ http://localhost:8080
2 threads and 10 connections
Thread Stats Avg Stdev Max /- Stdev
Latency 99.96us 100.03us 6.65ms 98.84%
Req/Sec 47.73k 2.43k 51.02k 89.11%
959308 requests in 10.10s, 117.10MB read
Requests/sec: 94978.71
Transfer/sec: 11.59MB
```
main:
```
Running 10s test @ http://localhost:8080
2 threads and 10 connections
Thread Stats Avg Stdev Max /- Stdev
Latency 163.03us 685.51us 19.73ms 99.27%
Req/Sec 39.50k 3.98k 66.11k 95.52%
789582 requests in 10.10s, 82.83MB read
Requests/sec: 78182.65
Transfer/sec: 8.20MB
```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracted from fast streams work.
This is a resource wrapper for
ReadableStream, allowing us to treat allReadableStreaminstances as resources, and remove special paths in bothfetchandserve.Performance with a ReadableStream response yields ~18% improvement:
This patch:
main: