feat(perf): implement libp2p perf protocol #3508

mxinden · 2023-02-26T14:34:56Z

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

Flow user -> client -> server -> client -> user is implemented.

thomaseizinger

Short drive-by review. Thanks for starting this!

thomaseizinger · 2023-02-27T01:36:18Z

protocols/perf/Cargo.toml

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


thomaseizinger · 2023-02-27T01:37:00Z

protocols/perf/examples/perf-client.rs

+    server_address: Multiaddr,
+}
+
+fn main() {


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".

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.

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

thomaseizinger · 2023-02-27T01:40:23Z

protocols/perf/src/client/behaviour.rs

+        // TODO: What if we are already connected?
+        self.queued_events
+            .push_back(NetworkBehaviourAction::Dial { opts });


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.

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

thomaseizinger · 2023-02-27T01:40:53Z

protocols/perf/src/client/handler.rs

+#[derive(Debug)]
+pub enum Event {
+    Finished { stats: RunStats },
+}
+
+#[derive(Debug)]
+pub enum Command {
+    Start { params: RunParams },
+}


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

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

thomaseizinger · 2023-02-27T01:41:18Z

protocols/perf/src/client/handler.rs

+        >,
+    ) {
+        match event {
+            ConnectionEvent::FullyNegotiatedInbound(_) => todo!(),


Nit: Use void::unreachable.

protocols/perf/src/protocol.rs

mxinden · 2023-03-07T14:23:18Z

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 · 2023-03-07T14:31:06Z

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`.

thomaseizinger

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

.dockerignore

thomaseizinger · 2023-03-08T08:43:36Z

protocols/perf/Cargo.toml

+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" }


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.

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.

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?

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

You could put them into misc/ too?

@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.

Sounds good.

protocols/perf/Cargo.toml

mergify · 2023-03-08T13:24:19Z

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

protocols/perf/tests/lib.rs

thomaseizinger · 2023-03-12T20:11:58Z

protocols/perf/Cargo.toml

+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" }


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?

mxinden

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

protocols/perf/Dockerfile

thomaseizinger

Looking very nice. Some non-blocking comments :)

protocols/perf/src/client/behaviour.rs

protocols/perf/src/server/behaviour.rs

thomaseizinger · 2023-03-13T15:05:19Z

protocols/perf/tests/lib.rs

+#[test]
+fn perf() {
+    let _ = env_logger::try_init();
+    let mut pool = LocalPool::new();


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

thomaseizinger · 2023-03-13T15:06:24Z

protocols/perf/tests/lib.rs

+        loop {
+            match client.select_next_some().await {
+                SwarmEvent::IncomingConnection { .. } => panic!(),
+                SwarmEvent::ConnectionEstablished { .. } => {}
+                SwarmEvent::Dialing(_) => {}
+                SwarmEvent::Behaviour(client::Event { result: Ok(_), .. }) => break,
+                e => panic!("{e:?}"),
+            }
+        }


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

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

Neat. Saves a bunch of lines of code.

protocols/perf/tests/lib.rs

thomaseizinger · 2023-03-13T15:09:10Z

Looking very nice. Some non-blocking comments :)

Feel free to merge whenever.

mxinden · 2023-03-14T10:16:09Z

Recommendations:

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

thomaseizinger · 2023-03-14T11:31:02Z

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

As we do with all other protocols, make sure to expose `libp2p-perf` as well. See also discussion libp2p#3508 (comment)

As we do with all other protocols, make sure to expose `libp2p-perf` as well. Related: #3508 (comment). Pull-Request: #3693.

mxinden added 11 commits February 25, 2023 14:30

Scaffolding

43a4f3e

Scaffolding for client protocol

d843caa

Implement client send receive

261922d

Scaffolding for end-to-end test

26e26d3

Implement server side protocol

a31e69a

Bubble events up to user

04858c6

Flow user -> client -> server -> client -> user is implemented.

Track stats for single run

e1a0ced

Add client and server example

a41f41b

Track time by write and read

a0a55ae

Don't allocate in protocol implementation

1c3bfe9

Don't panic on stream negotiation failure

b95f8a6

mxinden mentioned this pull request Feb 26, 2023

Introduce perf spec libp2p/specs#478

Merged

Add support for QUIC

9bbd99a

thomaseizinger reviewed Feb 27, 2023

View reviewed changes

p-shahi mentioned this pull request Feb 28, 2023

Future-proof Benchmarking ⏱ libp2p/test-plans#63

Open

26 tasks

mxinden added 12 commits March 3, 2023 19:12

Fix crate description

761df78

Move examples into src/bin/

3b68b37

Handle inbound connection in client behaviour

ffdf089

Don't do connection management in perf behaviour

39694af

Adjust tests and don't depend on DummyHandler

4cab07d

Implement keep alive handling

b9ef80f

Have server emit event with run stats

489fbfe

Handle outbound error on client handler

96e7997

Handle listen error in client handler

27586dc

Handle stream errors on server handler

3fbd70e

Handle FromSwarm events in client behaviour

8b52690

Adjust tests

d9ee033

mxinden marked this pull request as ready for review March 4, 2023 14:44

mxinden added 2 commits March 4, 2023 15:44

Merge branch 'master' of https://github.com/libp2p/rust-libp2p into perf

8c43498

Fix doc comment

ded45c5

mxinden added 2 commits March 7, 2023 15:23

Merge branch 'master' of https://github.com/libp2p/rust-libp2p into perf

82eb107

Fix clippy lint

b14f6b8

Don't expose Handler publicly

5f3e382

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`.

thomaseizinger reviewed Mar 8, 2023

View reviewed changes

thomaseizinger reviewed Mar 12, 2023

View reviewed changes

mxinden added 4 commits March 13, 2023 14:35

Remove .dockerignore diff

ba96227

Merge branch 'master' of https://github.com/libp2p/rust-libp2p into perf

64b6498

Use libp2p-swarm-test

9519562

Add newline to Dockerfile

3b3b08f

mxinden commented Mar 13, 2023

View reviewed changes

protocols/perf/Dockerfile Outdated Show resolved Hide resolved

thomaseizinger reviewed Mar 13, 2023

View reviewed changes

mxinden added 2 commits March 19, 2023 17:56

Use async_std::test and libp2p-swarm-test wait

ea30c1b

Limit the number of decimals for floats in output

a98ca38

thomaseizinger approved these changes Mar 19, 2023

View reviewed changes

mxinden added 3 commits March 19, 2023 18:41

Log start and connection establishment

cd846b1

Merge branch 'master' of https://github.com/libp2p/rust-libp2p into perf

2651d72

Add changelog

75065bd

mxinden added the send-it label Mar 19, 2023

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

fix(libp2p): Export perf protocol

cd14e18

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

fix(libp2p): Export perf protocol

cdbae2c

As we do with all other protocols, make sure to expose `libp2p-perf` as well. See also discussion libp2p#3508 (comment)

mxinden mentioned this pull request Mar 29, 2023

fix(libp2p): export perf protocol #3693

Merged

4 tasks

mergify bot pushed a commit that referenced this pull request Mar 29, 2023

fix(libp2p): export perf protocol

3d3ccfe

As we do with all other protocols, make sure to expose `libp2p-perf` as well. Related: #3508 (comment). Pull-Request: #3693.

mxinden mentioned this pull request Apr 24, 2023

deps: bump clap from 4.2.1 to 4.2.4 #3808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(perf): implement libp2p perf protocol #3508

feat(perf): implement libp2p perf protocol #3508

mxinden commented Feb 26, 2023 •

edited

thomaseizinger left a comment

thomaseizinger Feb 27, 2023

thomaseizinger Feb 27, 2023

mxinden Feb 27, 2023

thomaseizinger Feb 27, 2023

thomaseizinger Feb 27, 2023

mxinden Mar 3, 2023

thomaseizinger Feb 27, 2023

mxinden Mar 3, 2023

thomaseizinger Feb 27, 2023

mxinden commented Mar 7, 2023

mxinden commented Mar 7, 2023

thomaseizinger left a comment

thomaseizinger Mar 8, 2023 •

edited

mxinden Mar 12, 2023

thomaseizinger Mar 12, 2023

mxinden Mar 13, 2023

thomaseizinger Mar 13, 2023

mxinden Mar 17, 2023

thomaseizinger Mar 17, 2023

mergify bot commented Mar 8, 2023

thomaseizinger Mar 12, 2023

mxinden left a comment

thomaseizinger left a comment

thomaseizinger Mar 13, 2023

thomaseizinger Mar 13, 2023

thomaseizinger Mar 13, 2023

mxinden Mar 19, 2023

thomaseizinger commented Mar 13, 2023

mxinden commented Mar 14, 2023

thomaseizinger commented Mar 14, 2023

feat(perf): implement libp2p perf protocol #3508

feat(perf): implement libp2p perf protocol #3508

Conversation

mxinden commented Feb 26, 2023 • edited

Description

Notes

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Mar 7, 2023

mxinden commented Mar 7, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Mar 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 8, 2023

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Mar 13, 2023

mxinden commented Mar 14, 2023

thomaseizinger commented Mar 14, 2023

mxinden commented Feb 26, 2023 •

edited

thomaseizinger Mar 8, 2023 •

edited