-
Notifications
You must be signed in to change notification settings - Fork 995
Improve performance of peerselector peers #9586
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
base: main
Are you sure you want to change the base?
Improve performance of peerselector peers #9586
Conversation
…useless responses for some invalid responses Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda-Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…r task feature toggle in BackwardSyncStep Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
|
Do you have times for BWS header download with and without your PR for comparison? |
|
Control node BWS header download was pretty consistant around 20 minutes. With these changes, they're both around the 20 minute mark. |
…uses memory leaks Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tor-peers' into improve-performance-of-peerselector-peers
…if it causes memory leaks" This reverts commit b9188d7. Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
0d1d1b9 to
8ef2bc5
Compare
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
pinges
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge if you have a good answer for my question :-)
| TOO_MANY_RESULTS_RETURNED(null), | ||
| RESULTS_DO_NOT_MATCH_QUERY(null), | ||
| NO_RESULTS_RETURNED(null, true), | ||
| TOO_MANY_RESULTS_RETURNED(null, false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we not record a useless response when there are too many results returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I was mostly just guessing. Assuming the results returned contains the results we expected, plus others, it's still useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code it looks like we are not making use of the response when the validation response is TOO_MANY_RESULTS_RETURNED. So I guess it should at least be a useless response.

PR description
Improve performance during backward sync by using bestPeerComparator. Also implement recording useless responses from peers in PeerTaskExecutor to eventually disconnect bad peers.