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

chore: dont use loop on yamux #307

Merged
merged 1 commit into from Feb 19, 2021
Merged

chore: dont use loop on yamux #307

merged 1 commit into from Feb 19, 2021

Conversation

driftluo
Copy link
Collaborator

@driftluo driftluo commented Feb 10, 2021

Rust's asynchronous system is a collaborative model, which means that all Futures should not occupy the executor for a long time.

There is a risk of preemption when yamux uses a loop. There will be no obvious problems under a single connection. However, when a large number of connections exist, if yamux is occupied for a long time, it will affect the scheduling of the entire task system, especially when the CPU capacity is insufficient. It will become more and more obvious on the machine, the consequence is that the communication speed will be greatly jittered.

What are the problems with occupying the executor for a long time?

First, it will cause the work-steal trigger to become more frequent
Second, it will affect the drivers triggering

Generally, Future tasks use executor in the principle of proximity, and bind on an executor, triggering work-steal means that there is additional synchronization work

Here we specify the maximum upper limit of one poll. If yamux still needs to be executed, first yield out, perform other tasks, or execute itself again

ref: tokio-rs/tokio#3493

@driftluo driftluo requested review from a team and quake February 10, 2021 03:00
yamux/src/session.rs Outdated Show resolved Hide resolved
yamux/src/session.rs Outdated Show resolved Hide resolved
@driftluo driftluo force-pushed the dont-use-loop branch 4 times, most recently from 71163cb to eb88c01 Compare February 10, 2021 14:32
yamux/src/session.rs Outdated Show resolved Hide resolved
@doitian doitian self-requested a review February 19, 2021 08:32
@driftluo driftluo merged commit 2cdd795 into master Feb 19, 2021
@driftluo driftluo deleted the dont-use-loop branch February 19, 2021 09:24
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.

None yet

3 participants