-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 fetch for chain sync #3887
Conversation
Marked as ready to review as it seems to be working; I'll wait for my node to do a full sync and figure out how long it takes. Please make suggestions for improvements, this is step 1. |
we were leaking streams right and left...
…concurrent sync requests
15f4783
to
2946561
Compare
rebased on master. |
|
||
mx.Lock() | ||
if requestResult != nil { | ||
copy(batch[j+offset:], requestResult) |
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.
I concurrent reslice and copy into a slice safe?
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.
It might be safe, I am just not sure -- hence the lock.
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.
Or do you mean the copy operation on the reslice? That should be perfectly fine, we make a new slice and then we copy.
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.
I 100% missed the lock ;p
fixed a minor issue in the end of sync, there was an off by 1 in the batch size. note that it didn't prevent from syncing, it just required an extra call to fetchMessages. |
running this i seem to get a lot of:
|
Yeah, that's kind of expected, we use more peers so at startup we might select some bad ones. |
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.
LGTM, and works well on my machine!
Also fixes stream leaks during sync.