libp2p / libp2p/rust-libp2p

Excessive calls to `poll` and cloning of `Waker`s.

Open
#4,990 7 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
5.6k
Forks
1.3k
Avg merge
8h 47m
Merged PRs (30d)
19

Description

Summary

I perfed my protocol and discovered major hot spots within the repo code mostly ending up in libp2p_swarm::connection::Connection<THandler>::poll and yamux implementation. From the looks of it the "nothing to do" paths are constantly being hit and subsequently the Arcs dropped and Wakers cloned. I quickly noticed that code usually tries to poll everything anytime one central Waker triggers.

Expected behavior

Ideally, code selecting many futures in parallel should branch the main Waker for each future to collect information on what actually needs polling.

Actual behavior

Most of the hot instructions are doing atomic operations within Waker::clone and code almost always looks like this:

pub fn set_waker(old: &mut Option<Waker>, new: &Waker) {
    *old = Some(new.clone());
}

The most significant hot spot though is something different. here we unconditionally create Arc for each available protocol as the last thing before breaking the loop. The crux of the problem is that we call the poll to drop some Arcs and clone Wakers most of the time with no real work getting done.

Relevant log output

No response

Possible Solution

Simplest fix for updating cached wakers is refactoring the function to:

pub fn set_waker(old: &mut Option<Waker>, new: &Waker) {
    if let Some(old) = old {
        // clone_from will clone only if `new` won't wake the `old`
        old.clone_from(new);
    } else {
        *old = Some(new.clone());
    }
}

Although this is just fixing the symptom, its still a substantial improvement considering how easy it is to implement.

Another simple fix is avoiding the Arc creation in mentioned section of the code. I managed to pull it off but not without breaking the API (set intersection and difference iterators getting replaced with SmallVecs in ProtocolChange events).

Now for the main cause of all this...

Excessive Polling

All the mentioned code is almost exclusively placed at spots where handwritten futures ran out of things to do, this is also the code that is executed all the time. My conclusion is that we poll when we don't need to. Solving this though is hard and requires lot of changes, and careful testing (since eliminating excessive polls is painful since bugs exhibit as program getting blocked forever).

Version

b7914e407da34c99fb76dcc300b3d44b9af97fac

Would you like to work on fixing this bug ?

Yes

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at swarm/src/connection.rs near Connection::poll and inspect the yamux implementation, including set_waker and the ProtocolChange handling described in the issue. Use the reported perf hotspots as a baseline, then verify that unnecessary polling, Waker cloning, and Arc creation are reduced without causing futures to stop making progress.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
distributed-systems, networking, performance
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.