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

WebSocketStream API #8831

Closed
crowlKats opened this issue Dec 19, 2020 · 9 comments · Fixed by #10365
Closed

WebSocketStream API #8831

crowlKats opened this issue Dec 19, 2020 · 9 comments · Fixed by #10365
Labels
ext/web related to the ext/web crate suggestion suggestions for new features (yet to be agreed)

Comments

@crowlKats
Copy link
Member

The WebSocketStream API is an API for usage of websockets using streams.
There is no formal spec for it yet.
https://web.dev/websocketstream/
https://docs.google.com/document/d/1XuxEshh5VYBYm1qRVKordTamCOsR-uGQBCYFcHXP4L0/edit#
https://docs.google.com/document/d/1La1ehXw76HP6n1uUeks-WJGFgAnpX2tCjKts7QFJ57Y/edit
https://github.com/ricea/websocketstream-explainer/
https://chromestatus.com/feature/5189728691290112

Would there be interest in having it? From discussions it seems the answer would be yes, including #7450 (comment)

@lin7sh
Copy link

lin7sh commented Dec 19, 2020

Thanks for bringing it up. And WebSocketStream will be enabled by default on Chrome 89. source

@crowlKats
Copy link
Member Author

I have a mostly-done implementation for it on a branch, but i am not sure if it is quite correct as there is no proper spec

@lin7sh
Copy link

lin7sh commented Dec 19, 2020

One question: Does the WHTWHG Stream implementation in Deno comes from chromium Stream implementation? There's new ByteStream feature in Chrome Stream

@kitsonk
Copy link
Contributor

kitsonk commented Dec 19, 2020

The stream implementation in Deno is built in JavaScript and is part of the op_web crate.

I'm not sure we would want to include something that is not part of the living standard until it is part of the living standard. Chromium has jumped the gun a number of times and it only ends in sadness.

@kitsonk kitsonk added ext/web related to the ext/web crate suggestion suggestions for new features (yet to be agreed) labels Dec 19, 2020
@lin7sh
Copy link

lin7sh commented Dec 20, 2020

@kitsonk Thanks for the information, I've also noticed WebKit's implementation also written is JS, I think it because of the stability result same as Deno. But I've noticed many new interesting features are going to be based on WHTWHG's Stream, besides WebSocketStream, there's also QUICTransport and RTCQuicTransport. There are still in flux, but all venders is heading to QUIC and HTTP3, and it will happen soon than we expected(probably before 2021Q3). So a correct and performant Steam implementation is a key enabler.

@kitsonk
Copy link
Contributor

kitsonk commented Dec 20, 2020

So a correct and performant Steam implementation is a key enabler.

What is incorrect or non-performant with the Deno Stream implementation?

@lin7sh
Copy link

lin7sh commented Dec 21, 2020

I haven't done any benchmark, but I think this level code should be done in c++/rust level

@lucacasonato
Copy link
Member

lucacasonato commented Dec 21, 2020

Well please do benchmarks before making claims regarding performance :-). I would not be surprised at all if a pure JS implementation of WHATWG streams is faster and has higher throughput (and especially has better tail latencies) than a comparable C++/Rust implementation due to the lower context switch overhead.

Also this is not how Deno is built. Deno does not bind objects / functions directly into V8, except for the infrastructure we need to do "ops" (deno syscalls). All code that does not need any privileged access (fs, net, env) is implemented in JS.

@kitsonk
Copy link
Contributor

kitsonk commented Dec 21, 2020

I agree with @lucacasonato that for streams and what they do, we are likely to have a higher performance in JavaScript. It really is a stated layer on top of isolate constructs anyways. Doing it in Rust (in the Deno CLI there is 0 C++, and the least amount of C++ when can get away with in rusty_v8) would be painful and likely end up in a less performant solution.

Additional streams being implemented, might have a stream source that is an op to Rust, but that it quite a bit different than implementing streams in Rust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext/web related to the ext/web crate suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants