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

feat(perf): implement libp2p perf protocol #3508

Merged
merged 56 commits into from
Mar 19, 2023
Merged

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Feb 26, 2023

Description

Implementation of the libp2p perf protocol according to libp2p/specs#478.

//CC @MarcoPolo as the author of the specification.

Don't (yet) expect this to produce reliable performance metrics.

Notes

Server implementation is deployed at /dns4/libp2p-perf.max-inden.de/tcp/4001.

Easiest way to test from your machine:

docker run -ti --rm --entrypoint perf-client mxinden/libp2p-perf --server-address /dns4/libp2p-perf.max-inden.de/tcp/4001

Finished run: Sent 10 MiB in 9.497557834 s with 8.423217989114063 MiBit/s and received 10 MiB in 6.494584597 s with 12.317954875351667 MiBit/s

For server machine metrics, see https://libp2p-perf.max-inden.de/d/rYdddlPWk/node-exporter-full

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Short drive-by review. Thanks for starting this!

name = "libp2p-perf"
edition = "2021"
rust-version = "1.62.0"
description = "Direct connection upgrade through relay"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it? :)

server_address: Multiaddr,
}

fn main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these examples and not proper crate binaries? I think the fact that we ship them in a Docker container makes them binary worth instead of just "examples".

Copy link
Member Author

Choose a reason for hiding this comment

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

My flawed thought process, moving everything to examples/ after starting with src/bin/, was the hope to prevent circular dependencies in Cargo.toml/[dependencies].

Thinking about it some more, as long as I don't depend on libp2p directly, I should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

My flawed thought process, moving everything to examples/ after starting with src/bin/, was the hope to prevent circular dependencies in Cargo.toml/[dependencies].

Thinking about it some more, as long as I don't depend on libp2p directly, I should be fine.

The other option would be to create an entirely separate crate, like interop-tests instead of building binaries within libp2p-perf.

I don't mind either way. With latter, you can easily depend on libp2p. With the former, you need to depend on select crates that you want to use.

protocols/perf/src/client/behaviour.rs Show resolved Hide resolved
Comment on lines 60 to 62
// TODO: What if we are already connected?
self.queued_events
.push_back(NetworkBehaviourAction::Dial { opts });
Copy link
Contributor

Choose a reason for hiding this comment

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

I've developed the opinion that protocols should not manage connections unless their sole purpose is to do so (like dcutr or kad).

Instead, I'd state it as a prerequisite that we are connected and use NotifyHandler::Any.

You can then establish the connection within the binary through Swarm::dial.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I share the same opinion. Changed with 39694af.

Comment on lines 35 to 43
#[derive(Debug)]
pub enum Event {
Finished { stats: RunStats },
}

#[derive(Debug)]
pub enum Command {
Start { params: RunParams },
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should they be enums if they don't have to be?

Copy link
Member Author

Choose a reason for hiding this comment

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

Once I have proper error handling, I expect more variants to be needed, at least for Event.

>,
) {
match event {
ConnectionEvent::FullyNegotiatedInbound(_) => todo!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Use void::unreachable.

protocols/perf/src/protocol.rs Show resolved Hide resolved
@mxinden mxinden marked this pull request as ready for review March 4, 2023 14:44
@mxinden
Copy link
Member Author

mxinden commented Mar 7, 2023

Great review. Thanks for the help @thomaseizinger! I will leave this open a bit longer as I posted some follow-up questions to your comments.

@mxinden
Copy link
Member Author

mxinden commented Mar 7, 2023

The wasm32-unknown-unknown CI step is currently failing. That makes sense as we don't feature gate libp2p-perf for non-wasm, but at the same time use crates like libp2p-tcp as a dependency.

I am leaning towards flagging this as a non-wasm crate for now. In case anyone wants to use it with wasm we can introduce a more advanced feature gating with the libp2p-tcp etc. dependencies.

Any objections?

The user doesn't need to know the implementation details of `Handler`.
The can refer to the type via `Behaviour::Handler` if they want and
the only API they should care about is that it implements `ConnectionHandler`.
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Couple more comments, plus I pushed a commit to reduce the API surface. Let me know if you disagree.

.dockerignore Outdated Show resolved Hide resolved
Comment on lines 20 to 26
libp2p-core = { version = "0.39.0", path = "../../core" }
libp2p-dns = { version = "0.39.0", path = "../../transports/dns", features = ["async-std"] }
libp2p-noise = { version = "0.42.0", path = "../../transports/noise" }
libp2p-quic = { version = "0.7.0-alpha.2", path = "../../transports/quic", features = ["async-std"] }
libp2p-swarm = { version = "0.42.0", path = "../../swarm", features = ["macros", "async-std"] }
libp2p-tcp = { version = "0.39.0", path = "../../transports/tcp", features = ["async-io"] }
libp2p-yamux = { version = "0.43.0", path = "../../muxers/yamux" }
Copy link
Contributor

@thomaseizinger thomaseizinger Mar 8, 2023

Choose a reason for hiding this comment

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

We only need most these for the binaries right? How about we introduce a separate crate that implements the server and client binaries, similarly to interop-tests? That would allow this protocol to be WASM-friendly. Plus, in the spirit of #3111, it would be consistent with not introduce binaries in our protocol crates.

Copy link
Member Author

Choose a reason for hiding this comment

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

Which folder should these binaries go into?

That would allow this protocol to be WASM-friendly.

I don't think we should introduce any complexity for the sake of supporting WASM at this point. It is not a critical protocol. I don't see it used in WASM any time soon.

Plus, in the spirit of #3111, it would be consistent with not introduce binaries in our protocol crates.

Consistency is a good argument in my eyes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which folder should these binaries go into?

You'd have to make another crate, like interop-tests. Perhaps perf-utils, perf-cs (for perf-client-server) or perf-bins?

Copy link
Member Author

Choose a reason for hiding this comment

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

And perf-bins/ would be a top level folder? Is that worth the noise it is introducing?

Copy link
Contributor

Choose a reason for hiding this comment

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

You could put them into misc/ too?

Copy link
Member Author

Choose a reason for hiding this comment

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

@thomaseizinger do you feel strongly about this? Thinking about it some more, I don't think the split into two folders is worth the effort at this point. I expect this crate to change quite a bit after this pull request merges.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

protocols/perf/Cargo.toml Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Mar 8, 2023

This pull request has merge conflicts. Could you please resolve them @mxinden? 🙏

protocols/perf/tests/lib.rs Outdated Show resolved Hide resolved
Comment on lines 20 to 26
libp2p-core = { version = "0.39.0", path = "../../core" }
libp2p-dns = { version = "0.39.0", path = "../../transports/dns", features = ["async-std"] }
libp2p-noise = { version = "0.42.0", path = "../../transports/noise" }
libp2p-quic = { version = "0.7.0-alpha.2", path = "../../transports/quic", features = ["async-std"] }
libp2p-swarm = { version = "0.42.0", path = "../../swarm", features = ["macros", "async-std"] }
libp2p-tcp = { version = "0.39.0", path = "../../transports/tcp", features = ["async-io"] }
libp2p-yamux = { version = "0.43.0", path = "../../muxers/yamux" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Which folder should these binaries go into?

You'd have to make another crate, like interop-tests. Perhaps perf-utils, perf-cs (for perf-client-server) or perf-bins?

Copy link
Member Author

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

All comments, apart from new location for the binaries addressed. @thomaseizinger mind taking another look?

protocols/perf/Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Looking very nice. Some non-blocking comments :)

protocols/perf/src/client/behaviour.rs Show resolved Hide resolved
protocols/perf/src/client/behaviour.rs Show resolved Hide resolved
protocols/perf/src/server/behaviour.rs Show resolved Hide resolved
#[test]
fn perf() {
let _ = env_logger::try_init();
let mut pool = LocalPool::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make it an async_std::test? I find .await nicer than function-calls to LocalPool.

Comment on lines 57 to 65
loop {
match client.select_next_some().await {
SwarmEvent::IncomingConnection { .. } => panic!(),
SwarmEvent::ConnectionEstablished { .. } => {}
SwarmEvent::Dialing(_) => {}
SwarmEvent::Behaviour(client::Event { result: Ok(_), .. }) => break,
e => panic!("{e:?}"),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to use client.wait here as it incorporates a timeout whereas select_next_some will wait forever, potentially making your test hang.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably return the result and then either assert!(result.is_ok) or unwrap/expect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Neat. Saves a bunch of lines of code.

protocols/perf/tests/lib.rs Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

Looking very nice. Some non-blocking comments :)

Feel free to merge whenever.

@mxinden
Copy link
Member Author

mxinden commented Mar 14, 2023

Recommendations:

  • Make send/receive bytes configurable.
  • Expose more information, e.g. when the connection is established.

@thomaseizinger
Copy link
Contributor

Recommendations:

  • Make send/receive bytes configurable.
  • Expose more information, e.g. when the connection is established.
  • Limit the number of decimals for floats in output

@mergify mergify bot merged commit 9d1116f into libp2p:master Mar 19, 2023
mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Mar 29, 2023
As we do with all other protocols, make sure to expose `libp2p-perf` as well.

See also discussion libp2p#3508 (comment)
mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Mar 29, 2023
As we do with all other protocols, make sure to expose `libp2p-perf` as well.

See also discussion libp2p#3508 (comment)
@mxinden mxinden mentioned this pull request Mar 29, 2023
4 tasks
mergify bot pushed a commit that referenced this pull request Mar 29, 2023
As we do with all other protocols, make sure to expose `libp2p-perf` as well.

Related: #3508 (comment).

Pull-Request: #3693.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants