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

Parallel gossiping #76

Closed
wants to merge 9 commits into from
Closed

Conversation

rahulghangas
Copy link
Contributor

As title

@rahulghangas
Copy link
Contributor Author

CI fails for now, will work with the fix in #73 after it is reviewed and merged

@ross-pure ross-pure changed the base branch from experiment/functional to release/0.5.0 March 24, 2021 01:05
@rahulghangas rahulghangas requested a review from jazg April 19, 2021 12:11
peer/gossip.go Outdated Show resolved Hide resolved
@ross-pure
Copy link
Member

ross-pure commented Apr 20, 2021

I'm not in favour of this change. This PR spawns each call to transport.Send in it's own go routine, but I think this is potentially less efficient than making the calls sequentially. All that transport.Send does in the case that the peer is already linked is write to a buffered channel, and if it is not linked then it calls dial in a go routine. In total this is not really enough work to warrant being done in parallel in my opinion.

I tested how long each gossip takes in the gossip test with and without this change, and on my laptop the times were indistinguishable.

EDIT: Forgot to mention that spawning go routines for each send is probably worse on the whole because of the additional resources required for the go routines and the unnecessary extra work they give to the scheduler.

@rahulghangas
Copy link
Contributor Author

rahulghangas commented Apr 20, 2021

@ross-pure this came after an issue we (I and Jaz) discussed regarding gossip timeouts. We opted for the parallel solution because if the first send call blocks (stuck in dial), it can potentially cause the other peers to not receive the gossiped message

@ross-pure
Copy link
Member

@rahulghangas But it looks like any potential call to dial is in a go routine already, specifically I'm looking at transport.go:160 and transport.go:168

@rahulghangas
Copy link
Contributor Author

You are right. In that case, there is indeed no need to parallelize the Send(..) calls. I'll close the issue

@rahulghangas rahulghangas deleted the feat/parallel-gossip branch April 20, 2021 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants