refactor(gossipsub): don't store messages within Arc

[thomaseizinger]

Description

Currently, we store messages to be sent to the ConnectionHandler in an Arc. However, we never actually clone these messages as we can see with this patch, hence we remove this wrapping.

Related: #3242

Notes

@AgeManning I think we talked about this at some point but I forget the reasoning for why this is done. Can you recall it?

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

[AgeManning]

So originally it came from here: #898 (comment)

I think the idea was that we were allocating GossipsubRpc messages for every peer we needed to send them to. The Arc was to avoid the allocation.

Looking at the current code it does seem like we are just cloning the raw GossipsubRpc. I believe at one point we were just cloning the Arc. See for example here: https://github.com/libp2p/rust-libp2p/blob/master/protocols/gossipsub/src/behaviour.rs#L539

I think somewhere along the line it has changed. The send_message() probably took an Arc and we cloned the Arc. Looks like now we are just allocating and the Arc is pointless.

I'm indifferent if we want to keep the Arc to avoid the allocation or just remove it all together. I think this has come up quite a few times in the past, so its probably better to remove it so we don't forget again in the future.

[thomaseizinger]

Thanks for digging up the conversation! I tried to check whether we are cloning the message some where by removing Clone from the event: https://github.com/libp2p/rust-libp2p/pull/3243/files#diff-4656833730e135d1bfaa19578445814deab8bbb2a315a526546810003aebf6a1L67

It compiled without errors which puzzled me initially but it makes sense if we are cloning the entire RPC message. I think I have an idea how to implement this in a more obvious way that isn't accidentally refactored away as easily.

[thomaseizinger]

I opted to revert the commits again. The tests were broken because they only check the events buffer and don't actually poll the behaviour. I figured it is not worth revising all these tests. Given that the optimisation was already "refactored away" in a previous iteration, this PR doesn't actually make anything worse other than removing the Arc and that is what I want / need / fell out of #3242.

[AgeManning]

Yeah sounds good to me.

The optimization was never benched and its dubious as to how much it is saving us.

[thomaseizinger]

CI is broken because of #3252.

[thomaseizinger]

Friendly ping @mxinden @jxs :)

[jxs]

LGTM!