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

Browser Dialer: Change from ES5 to ES6+ for performance #3832

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

PoneyClairDeLune
Copy link
Contributor

@PoneyClairDeLune PoneyClairDeLune commented Sep 19, 2024

First of all... What. The. Heck.

Problems with the current code

I can only do so much to try getting the current code up to shape.

A mix of ES5 and ES6+ syntax in compatible mode

I really don't know if the browser dialer was designed to work on Internet Explorer, but proper web stream API support don't really exist on that platform. So why didn't the browser dialer have strict mode enabled when this could bring performance gains...

Unscoped value declarations with var

TL;DR, avoid using var at all costs. var introduces problems of the yesteryear, where let declarations are much safer, cleaner and straightforward, with stricter constraints allowing the JS JIT to work more efficiently.

Uses if-else chains instead of switch

I don't know why if-else chains are used instead of switch, as this isn't either C or C++. Both JS and Go support using switch on any type, and JS in specific has single-pass optimizations for switch statements, avoiding sequentially querying for each possibility.

Fundamental design flaws

The problems

I have no idea why the data plane and the control plane are smashed together. Evident in the code that each message of the single local WebSocket connection is parsed, where with a separation of data plane and control plane can avoid most of the processing. Unless it's tuned for the individual upload packets SplitHTTP needs, so in this specific case data could be smashed together. Since WebSocket can overwhelm the browser easily with enough throughput and without any counteracting measure, the only solution for now would be get rid processing as much as possible on the data plane.

And somehow the browser dialer of SplitHTTP is implemented via WebSocket instead of fetch request in either direction. At least on Chrome, ReadableStream could be set as the body, so instead of reading and processing chunks, bodies can simply be passed along without the browser doing much at all. Even better, since Chrome has streaming support for WebSocket in the latest versions, a zero-cost optimization is incredibly feasible with separated control and data planes.

A probably better design

Feel free to conduct critique on this, as this isn't fleshed out much yet.

/control?token=csrfToken

Control socket. Where the control messages should go, be it WS , GET or POST. Though non-streamed uploads should go here as well to reduce the overhead.

/ws?socket=aRandomId

Direct WebSocket message pass-through between the server and the client.

/http?socket=aRandomId&role=ingress

Where the magic for streamed web request happens. Allows stream pass-through without any manual reading, unless it's on Firefox.

Because the browser doesn't support h2c, two request streams for each client-facing streamed SplitHTTP connection are required. ingress streams are what the Xray server sends to the client, and egress streams are what the client sends to the server.

@PoneyClairDeLune PoneyClairDeLune marked this pull request as ready for review September 19, 2024 06:30
@PoneyClairDeLune PoneyClairDeLune changed the title Change browser dialer syntax from ES5 (Internet Explorer) to ES6+ (modern) for performance Changes browser dialer syntax from ES5 to ES6+ for performance Sep 19, 2024
@mmmray mmmray changed the title Changes browser dialer syntax from ES5 to ES6+ for performance Browser Dialer: Change from ES5 to ES6+ for performance Sep 19, 2024
@mmmray
Copy link
Collaborator

mmmray commented Sep 19, 2024

I agree with everything you are saying. The mixed style is mostly because of historical reasons and because nobody here really knows idiomatic JS (is my guess...). With regard to the redesign, I think that if it makes the JS code simpler, it's a win. I think the golang code will probably become more complicated, but you can try it. It would be particularly good to get rid of the JS loop in the GET codepath, as it is the source of some kind of resource leak.

The PR works on my machine, so I'll merge it now. Thanks for the PR, looking forward to the next one!

@mmmray mmmray merged commit 7677ac9 into XTLS:main Sep 19, 2024
@RPRX
Copy link
Member

RPRX commented Sep 20, 2024

And somehow the browser dialer of SplitHTTP is implemented via WebSocket instead of fetch request in either direction. At least on Chrome, ReadableStream could be set as the body, so instead of reading and processing chunks, bodies can simply be passed along without the browser doing much at all. Even better, since Chrome has streaming support for WebSocket in the latest versions, a zero-cost optimization is incredibly feasible with separated control and data planes.

可以改一下,实在不行就改名 chrome dialer

@RPRX
Copy link
Member

RPRX commented Sep 20, 2024

Chrome has streaming support for WebSocket in the latest versions

这是否意味着 browser dialer 可以支持 HTTPUpgrade 了?

@PoneyClairDeLune
Copy link
Contributor Author

Does this mean browser dialer can support HTTPUpgrade?

No, it cannot. Since WebSocket can overwhelm or crash clients with enough throughput, WebSocketStream was provided to handle streams with native backpressure support. It's still fully-fledged WebSocket at its core, just optimized for critical workloads and so happens that it perfectly fits the use case of a browser dialer.

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.

3 participants