-
Notifications
You must be signed in to change notification settings - Fork 43
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): add (provision, build, run) tooling #163
Conversation
See `perf/README.md`.
Test run between server in us-west-1 and client in us-east-1: Note that this is still using t2.micro machines, in other words these numbers are not representative.
|
Recent commits add a runner implementation, running a Rust Perf docker image on the AWS client once with TCP, once with QUIC:
|
Status Update:
|
Very cool! |
Where is the rust implementation? |
Rust implementation is here: |
Pushed a Go implementation, could you try it out? I'm curious if we'll see the |
- Makes emited values self-describing. - Ensures accounting of connection establishment. - Allows differentiation in subsequent data analysis.
Agreed we should have something very comparable to HTTPs, but can we also add other security protocol negotiation so we can see how much that impacts performance? |
Use TLS only instead of both TLS and Noise. Removes the additional multistream-select security protocol negotiation. Thus makes it easier to compare with TCP+TLS+HTTP/2
Makes the JSON easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcoPolo @marten-seemann as discussed before I added patches to the go-libp2p perf implementation to not wait for the libp2p identify exchange when (1) establishing the connection and (2) opening the perf stream.
There is likely a more go-libp2p idiomatic way of doing this. Feedback welcome.
start := time.Now() | ||
h.Peerstore().AddAddrs(serverInfo.ID, serverInfo.Addrs, peerstore.TempAddrTTL) | ||
// Use h.Network().DialPeer() instead of h.Connect to skip waiting for | ||
// identify protocol to finish. | ||
_, err = h.Network().DialPeer(context.Background(), serverInfo.ID) | ||
if err != nil { | ||
panic(err) | ||
} | ||
connectionEstablished := time.Since(start) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping the identify wait in Connect
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marten-seemann @MarcoPolo @sukunrt any idea why the connection establishment with the code above takes 3 network round-trips? I would expect it to take 2 RTT.
My assumption:
- 1 RTT for TCP.
- I only enable TLS, thus no security protocol negotiation, more specifically optimistic 0RTT multistream-select.
- 1 RTT for TLS. (Assuming TLS 1.3 is the default.)
- Using default muxers, which should be only Yamux, thus no muxer negotiation, more specifically optimistic 0RTT multistream-select.
- Not waiting for the additional identify exchange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-libp2p will spend 1RTT negotiating the security protocol.
go-libp2p doesn't do 0RTT select for security protocol. It does 0RTT select for other protocols when it knows the peer supports the given protocol otherwise it'll still do the 1RTT select.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-libp2p will spend 1RTT negotiating the security protocol.
Given that the client only supports one security protocol, i.e. TLS, shouldn't it be using the lazy mechanism, in other words optimistically use TLS @sukunrt?
https://github.com/multiformats/go-multistream/blob/master/lazyClient.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can change this. But currently this is not what's happening.
https://github.com/libp2p/go-libp2p/blob/master/p2p/net/upgrader/upgrader.go#L319-L330
In security negotiation we don't use lazyClient.
It uses this function which does wait for the reply. https://github.com/multiformats/go-multistream/blob/master/client.go#L117
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @sukunrt. I forgot that go-libp2p supports the multistream-select simultaneous-open extension and thus can not do an optimistic multistream-select negotiation when negotiating a single protocol (here TLS) only.
The extension is not needed in libp2p's hole punching as roles (i.e. dialer and listener) are derived from the roles on the relayed connection, see DCUtR protocol section. As far as I can tell the simultaneous-open extension is only needed for the case where two nodes randomly connect via TCP at the same time.
In my eyes the probability for two nodes to randomly, i.e. not coordinated via DCUtR, connect via TCP at the same time is low and only leads to a connection failure. Thus we don't support it in rust-libp2p.
@sukunrt @MarcoPolo @marten-seemann what do you think of removing the simultaneous-open extension in go-libp2p and thus enabling the optimistic security protocol negotiation when negotiation a single protocol only? This would allow go-libp2p to be able to compete with vanilla TCP+TLS+HTTP when it comes to connection establishment latency. In other words both go-lib2p with TLS+Yamux and vanilla TCP+TLS+HTTP would take 3 RTT (2 RTT connection establishment + 1 RTT request/response).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think we should make code changes just to look better in benchmarks. In practice, most configurations (IPFS and Lotus) use TLS and Noise.
@vyzo had numbers on simultaneous opens happening in the wild. Would you mind sharing them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, in nim-libp2p, we don't bind when we are public, which avoids uncoordinated simultaneous opens in the wild
We don't have enough data yet to see if it causes issues, but so far the only scenario where it doesn't work is if someone made a manual port mapping to a different port than the internal one, and didn't specify it to libp2p. In this case, we can't discover our public port, and we think we are private (since we don't bind)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the input everyone.
I documented the above as well as past discussions in libp2p/go-libp2p#2330. Let's continue the discussion over there in case there is interest.
// Use ps.Host.Network().NewStream() instead of ps.Host.NewStream() to | ||
// skip waiting for identify protocol to finish. | ||
s, err := ps.Host.Network().NewStream(network.WithNoDial(ctx, "already dialed"), p) | ||
if err != nil { | ||
return 0, 0, err | ||
} | ||
s.SetProtocol(ID) | ||
lzcon := msmux.NewMSSelect(s, ID) | ||
s = &streamWrapper{ | ||
Stream: s, | ||
rw: lzcon, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping the identify wait in NewStream
.
The above brings us one step closer to reaching https latency performance. The vanilla Go https implementation needs 3RTT, both rust-libp2p and with the above now also go-libp2p with TCP need 4 RTT. See https://observablehq.com/@mxinden-workspace/libp2p-perf#cell-621. |
We can. I suggest doing so in future iterations. Added to the list of potential next steps in the pull request description. |
Continued in #184.