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.

Sign up for GitHub

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

Jump to bottom

rewrite graph struct to be 200 times faster #3882

Merged
merged 54 commits into from
Feb 4, 2021
Merged

rewrite graph struct to be 200 times faster #3882

merged 54 commits into from
Feb 4, 2021

Conversation

pmnoxx
Copy link
Contributor

@pmnoxx pmnoxx commented Feb 2, 2021 •
edited
Loading

It looks like it's possible to rewrite graph class to be 200 times faster. Do we really need to move computation to another thread?

@pmnoxx pmnoxx mentioned this pull request Feb 2, 2021
Closed
matklad
matklad reviewed Feb 2, 2021
View reviewed changes
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
@pmnoxx pmnoxx force-pushed the piotr-rewrite-graph branch 2 times, most recently from efaa44b to 2888a01 Compare February 2, 2021 14:09
@pmnoxx pmnoxx self-assigned this Feb 2, 2021
@pmnoxx pmnoxx linked an issue Feb 2, 2021 that may be closed by this pull request
Update routing table in a separate thread #3853
Closed
@pmnoxx pmnoxx marked this pull request as ready for review February 2, 2021 14:19
@pmnoxx pmnoxx requested a review from mfornet as a code owner February 2, 2021 14:19
@pmnoxx pmnoxx changed the title rewrite graph struct to be 25 times faster rewrite graph struct to be 36 times faster Feb 2, 2021
matklad
matklad reviewed Feb 2, 2021
View reviewed changes
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
@pmnoxx pmnoxx force-pushed the piotr-rewrite-graph branch 3 times, most recently from 2921ebd to 4a2c741 Compare February 2, 2021 15:07
@pmnoxx pmnoxx marked this pull request as draft February 2, 2021 15:09
@pmnoxx pmnoxx marked this pull request as ready for review February 2, 2021 15:10
@pmnoxx pmnoxx requested a review from matklad February 2, 2021 20:07
@pmnoxx pmnoxx marked this pull request as draft February 2, 2021 22:14
bowenwang1996
bowenwang1996 reviewed Feb 3, 2021
View reviewed changes
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
utils/near-performance-metrics/src/stats_enabled.rs Outdated Show resolved Hide resolved
utils/deepsize/Cargo.toml.orig Outdated Show resolved Hide resolved
@pmnoxx pmnoxx force-pushed the piotr-rewrite-graph branch 5 times, most recently from d504147 to 928af46 Compare February 3, 2021 00:55
@pmnoxx pmnoxx marked this pull request as ready for review February 3, 2021 00:57
@pmnoxx pmnoxx changed the title rewrite graph struct to be 36 times faster rewrite graph struct to be 100 times faster Feb 3, 2021
matklad
matklad reviewed Feb 3, 2021
View reviewed changes
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Mostly nits, but total_active_edges seems like it might affect correctness.

Also, 100X perf improvement is nice :D

Also, I am wondering, does it make sense to try petgraph here? Although I myself also actually prefer to code simple graph routines myself.

chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
chain/network/src/routing.rs Outdated Show resolved Hide resolved
bowenwang1996
bowenwang1996 approved these changes Feb 4, 2021
View reviewed changes
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

looks good to me. @mfornet can you also review?

chain/network/src/routing.rs Show resolved Hide resolved
chain/network/src/routing.rs Show resolved Hide resolved
@pmnoxx pmnoxx requested a review from matklad February 4, 2021 16:17
@near-bulldozer near-bulldozer bot merged commit 833e230 into master Feb 4, 2021
@near-bulldozer near-bulldozer bot deleted the piotr-rewrite-graph branch February 4, 2021 18:10
bowenwang1996 pushed a commit that referenced this pull request Feb 18, 2021
@pmnoxx @bowenwang1996
rewrite graph struct to be 200 times faster (#3882)
4ea58d9 
It looks like it's possible to rewrite graph class to be 200 times faster. Do we really need to move computation to another thread?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowenwang1996 bowenwang1996 bowenwang1996 approved these changes

@mfornet mfornet Awaiting requested review from mfornet

@olonho olonho Awaiting requested review from olonho

@willemneal willemneal Awaiting requested review from willemneal

@matklad matklad Awaiting requested review from matklad

Assignees

@pmnoxx pmnoxx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update routing table in a separate thread
3 participants
@pmnoxx @matklad @bowenwang1996