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

feat: simplify kademlia topology build up #2427

Merged
merged 4 commits into from
Aug 31, 2021
Merged

feat: simplify kademlia topology build up #2427

merged 4 commits into from
Aug 31, 2021

Conversation

istae
Copy link
Member

@istae istae commented Aug 18, 2021

PR removes the overSaturationPeers check in connectNeighbours, and the second iterator neighborhood connector. We have more PRs up now that prunes oversaturated bins (which this PR causes to happen) and more optimizations to increase build up time.

This change is Reviewable

@istae istae requested review from mrekucci, acud and metacertain August 18, 2021 15:21
@istae istae added the ready for review The PR is ready to be reviewed label Aug 18, 2021
Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looks good but some code this PR deletes (e.g.: in the connectNeighbours) were quick & dirty optimization done before the 1.0 release which took topology buildup on our staging from ~10 min to ~2-3 min. Were these changes benchmarked for regression?

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @acud and @metacertain)


pkg/topology/kademlia/kademlia.go, line 315 at r1 (raw file):

		if po < depth {
			return false, true, nil

Unacassarry new line.

Copy link
Member Author

@istae istae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We benchmarked this on the main-net and there was no significant difference in build up time. We have a new branch now which optimizes kademlia on top of this simplification which should result in quicker build up compared to master.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @acud and @metacertain)

@istae
Copy link
Member Author

istae commented Aug 25, 2021

Some notes about the tests for this PR:
We ran all kademlia related branches, and master branch on the main-net, on bee-storage node 0, with preexisting state-store.

The /topology endpoint was queried on a 30 second interval for 20 times for each branch:

On simplify-kad branch: depth 12 was reached by min ~5
On prune-outofdepth branch: depth 12 was reached by min ~5
On balancer-opt branch: depth 11 was reached by min ~5

On master branch: depth 11 was reached by min ~4

On the master run for example, depth 12 was never reached, because of small population size in bin 12.

prune.zip
opt.zip
master.zip
simplify.zip

@istae istae merged commit 787e66f into master Aug 31, 2021
@istae istae deleted the simplify-kad branch August 31, 2021 17:52
@acud acud added this to the v1.2.0 milestone Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants