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

fix performance issue following PR 1705 #1709

Merged

Conversation

Projects
None yet
3 participants
@jeanphilippeD
Copy link
Contributor

commented Jul 10, 2019

Closes #1684

Because of additional messages sent, soak test experienced a performance hit.
Remove main bottle neck for performance that came from previous way of establishing trust that is disabled currently and will be replace by Secure Message Delivery.

perf/routing: not add proving sections not needed until SMD
Secure Message Delivery will replace the ProvingSection mechanism
with an alternative. For now we do not use them only append them
to all messages.

Disable adding them to messages as they take considerable memory.
There is also a significant CPU overhead since we are voting on them.

Performance of churn: 60 seconds with PR 1705, 14 second now.

@jeanphilippeD jeanphilippeD requested a review from fizyk20 Jul 10, 2019

@jeanphilippeD

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Added new commit to fix soak test failure.

fix/routing: not validate given ProvingSection before trusting them
SMD will ensure we have trust.
Currently trust is mostly disabled, so if we have a ProvingSection
going through parsec, we will vote for that SectionInfo in our neighbour
if it is a newer version.

Fix concurrent_merge seed Some([2044325391, 837965916, 820907250, 664787039])

@jeanphilippeD jeanphilippeD force-pushed the jeanphilippeD:issue_1684_followup_perf branch from 889cb26 to 112b462 Jul 10, 2019

@jeanphilippeD jeanphilippeD requested review from fizyk20 and maqi Jul 10, 2019

@jeanphilippeD jeanphilippeD force-pushed the jeanphilippeD:issue_1684_followup_perf branch from 112b462 to 84a2f6d Jul 11, 2019

@jeanphilippeD jeanphilippeD changed the title WIP: fix performance issue following issue 1684 fix performance issue following PR 1705 Jul 11, 2019

@jeanphilippeD

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Rebase and updated PR comment and title.

}
// Always add section info until SMD is complete

This comment has been minimized.

Copy link
@maqi

maqi Jul 11, 2019

Member

shall we have a TODO keyword here to highlight?

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Jul 11, 2019

Author Contributor

I prefer to be cautious with TODO as they quickly become outdated, essentially the TODO would be implement SMD.
We will definitely need to update this code for SMD, otherwise test that validate that we reject invalid update should fail.

This comment has been minimized.

Copy link
@maqi

maqi Jul 11, 2019

Member

I suggest to use a keyword of TODO just for a highlight so we won't forget to update this place of code (and the comment as well).
If you think it won't be forgot for sure, I am ok with leave it. :)

Show resolved Hide resolved src/states/elder/mod.rs

@jeanphilippeD jeanphilippeD dismissed stale reviews from maqi and fizyk20 via dabbced Jul 11, 2019

@jeanphilippeD jeanphilippeD force-pushed the jeanphilippeD:issue_1684_followup_perf branch from 84a2f6d to dabbced Jul 11, 2019

@jeanphilippeD

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Added TODO comment.

@maqi

maqi approved these changes Jul 11, 2019

@jeanphilippeD jeanphilippeD merged commit 9294479 into maidsafe:fleming Jul 11, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jeanphilippeD jeanphilippeD deleted the jeanphilippeD:issue_1684_followup_perf branch Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.