inject_connected and inject_disconnected might introduce small inefficiencies in NetworkBehaviour implementers

[divagant-martian]

Checking the implemented protocols (gossipsub, kad, relay, req/resp and floodsub, mdns and identify) all but floodsub and mdns implement at least a pair of the coupled methods: inject_connection_(closed|establised) and the corresponding inject_(disconnected|connected) from the NetworkBehaviour trait.

This seems to be because the protocols require pieces of information that are not given together: i.e connection direction and whether this connection is the last/first. In the general case the means the protocol implementing the trait retrieves the same Peer_id twice for every first connection and final disconnection. This could be avoided removing inject_disconnected and inject_connected and simply passing the number of connections as a parameter. If a protocol cares to check for the conditions for disconnections/connections it would mean to move the check that the swarm already does about the number of connections to the protocol.

[mxinden]

I don't think the fact that we have both inject_connection_(closed|establised) and inject_(disconnected|connected) is an inefficiency in a performance sense. I would argue that the cost of two function calls is negligible compared to the cost of establishing the connections themselves. Especially as inject_(disconnected|connected) involves a PeerId only, which should fit a single CPU cache line. Could you expand on which logic you would expect to be a hot-path?

I do agree that having both inject_connection_(closed|establised) and inject_(disconnected|connected) is non-intuitive from a user-experience perspective.

[divagant-martian]

implementation wise, protocols typically store information in a HashMap. Two calls = two searches

[divagant-martian]

fixed title to make the point more clear.

On one hand it may be a bit confusing the existence of both methods; but beyond this it also splits the logic required to handle new and closed connections/peers in a two step process in each protocol. Doing what protocols require to "unify" back this logic is what typically will produce a small inefficiency. I also see as an advantage to removing the inject_disconnected/connected methods we no longer needing to state the promise/contract about when is each method called with respect to the other. Some protocols actually have checks in place for this.

It also aligns nicely with SwarmEvent::ConnectionEstablished

[thomaseizinger]

On one hand it may be a bit confusing the existence of both methods

This has historical reasons because originally, rust-libp2p did not support multiple connections to the same peer and hence only inject_connected and inject_disconnected were needed.

[mxinden]

@divagant-martian do you want to propose a patch combining the four methods into two?

[divagant-martian]

That's my suggestion. If you ask if I would be giving a PR for this, yes. But I'd like to first gather thoughts on the matter. I guess that's the reason for opening this issue