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.

Sign up for GitHub

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

Jump to bottom

perf(dgw): use a buffer of 1k bytes for ARD VNC sessions #807

Closed
wants to merge 7 commits into from
Closed

perf(dgw): use a buffer of 1k bytes for ARD VNC sessions #807

wants to merge 7 commits into from

Conversation

RRRadicalEdward
Copy link
Contributor

ARD uses MVS video codec which doesn't like buffering, and we need to have the buffer as minimal as possible.

Also, this commit adds new copy_bidirectional transport that is forked the one from tokio. It's forked because the original function doesn't allow overriding the buffer size (8K is used by default). There is an issue on tokio side for it. We will be able to replace our fork with the upstream easily when it's ready.

Releated to https://github.com/Devolutions/IronVNC/issues/338.

@RRRadicalEdward
fix: Use 1024 buffer size for ARD VNC session
206fecd 
_ARD_ uses _MVS_ video codec which doesn't like buffering, and we need to have the buffer as minimal as possible.

Also, this commit adds new `copy_bidirectional` transport that is forked [the one from tokio](https://docs.rs/tokio/latest/tokio/io/fn.copy_bidirectional.html).
It's forked because the original function doesn't allow overriding the buffer size (8K is used by default). There is [an issue](tokio-rs/tokio#6454) on tokio side for it. We will be able to replace our fork with the upstream easily when it's ready.

Releated to Devolutions/IronVNC#338.
@RRRadicalEdward
fix(ci): run cargo fmt
3c59de0
@RRRadicalEdward
Copy link
Contributor Author

I needed to fork DevolutionsGateway because I don't have the right to push directly to the repo.

@CBenoit CBenoit changed the title fix: Use 1024 buffer size for ARD VNC session fix: use a buffer of 1k bytes for ARD VNC sessions Apr 15, 2024
@RRRadicalEdward
Copy link
Contributor Author

RRRadicalEdward commented Apr 15, 2024 •
edited

It's easy to add copy_bidirectional with the buffer size in tokio directly, but we haven't agreed on what the name should be. If you have any thoughts, I would appreciate 😄 - tokio-rs/tokio#6454.

@CBenoit CBenoit changed the title fix: use a buffer of 1k bytes for ARD VNC sessions perf: use a buffer of 1k bytes for ARD VNC sessions Apr 15, 2024
@CBenoit CBenoit changed the title perf: use a buffer of 1k bytes for ARD VNC sessions perf(dgw): use a buffer of 1k bytes for ARD VNC sessions Apr 15, 2024
@RRRadicalEdward @CBenoit
Update crates/transport/src/copy_bidirectional.rs
9a3c01f 
Co-authored-by: Benoît Cortier <bcortier@proton.me>
CBenoit
CBenoit approved these changes Apr 15, 2024
View reviewed changes
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

LGTM!! Thank you!

@CBenoit CBenoit enabled auto-merge (squash) April 15, 2024 07:39
@CBenoit
Copy link
Member

CBenoit commented Apr 15, 2024

For changelog:

Apple ARD uses the so-called MVS video codec.
It is a tricky codec: Apple didn't implement proper congestion control, so it's basically just TCP controlling the flow (not by much).
Our MVS implementation for the web client is obviously not as fast as the native one, and can’t keep up when there are too much data in transit.
To reduce the amount of data in transit, we reduced the size of the copy buffer when using web socket forwarding endpoint and if the application protocol of the session is set to ARD.

Issue: DGW-138

auto-merge was automatically disabled April 15, 2024 07:39

Head branch was pushed to by a user without write access

@CBenoit CBenoit enabled auto-merge (squash) April 15, 2024 07:39
@RRRadicalEdward
Copy link
Contributor Author

For changelog:

Apple ARD uses the so-called MVS video codec.
It is a tricky codec: Apple didn't implement proper congestion control, so it's basically just TCP controlling the flow (not by much).
Our MVS implementation for the web client is obviously not as fast as the native one, and can’t keep up when there are > too much data in transit.
To reduce the amount of data in transit, we reduced the size of the copy buffer when using web socket forwarding endpoint and if the application protocol of the session is set to ARD.

Issue: DGW-138

Perfect 👌

@CBenoit
Copy link
Member

CBenoit commented Apr 15, 2024

I’m sorry. It appears we can’t merge PRs from forks because of the new CI job which requires secrets (devolutions-gateway-web-ui). I’ll merge the code with this PR instead: #809

@CBenoit CBenoit closed this Apr 15, 2024
auto-merge was automatically disabled April 15, 2024 08:02

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CBenoit CBenoit CBenoit approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@RRRadicalEdward @CBenoit