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

Improve IPNS publish time #7

Merged
merged 14 commits into from
May 3, 2022
Merged

Improve IPNS publish time #7

merged 14 commits into from
May 3, 2022

Conversation

makew0rld
Copy link
Collaborator

@makew0rld makew0rld commented Apr 25, 2022

See commit messages for info. Maybe browse by commit because entire modules are brought in and are mostly unmodified.

- Time measuring was fixed to actually work
- Only 5 out of 20 peers are put to synchronously
- No follow up when querying peers for GetClosestPeers - for PutValue only

That last one was recommended by Adin and made the biggest difference,
eliminating the majority of the publishing time..
This reverts commit 47dc901.
// name publish operations
// Only publish to 5 out of 20 peers synchronously, the rest async

syncN := 5
Copy link
Member

Choose a reason for hiding this comment

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

Do we even need the 5 peers to be sync? Is it so we can catch some errors?

Would it be possible to have all async requests and just wait for the first 5 responses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They could all be async, since we have pubsub. I thought it made sense to at least send to some peers, so that when the API request completes you know it's gone out to the DHT to at least some degree. Idk.

Would it be possible to have all async requests and just wait for the first 5 responses?

As I understand it, the only response in this scenario is just an acknowledgement that the publish message was received. It's possible there is no acknowledgement at all, but I think there is. So by synchronously sending the publish message to 5 peers, we're doing that already.

Copy link
Member

Choose a reason for hiding this comment

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

As I understand it, the only response in this scenario is just an acknowledgement that the publish message was received. It's possible there is no acknowledgement at all, but I think there is. So by synchronously sending the publish message to 5 peers, we're doing that already.

What I was getting at is that doing it synchronously means that it's happening serially and we need to wait for each response to come in. But what we could do instead is do it in parallel and can get the responses out of order so that we can speed things up. E.g., each request happens in its own goroutine which writes to a channel on finish, then we listen to the first five responses and ignore the rest.

Is an individual synchronous request-response cycle fast enough that such an optimization wouldn't yield a big speedup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok so that is already happening. The message to each of the 5 peers is happening in its own goroutine. The "synchronous" part was that we wait for all 5 to complete before returning, but maybe that's a misnomer.

Copy link
Member

Choose a reason for hiding this comment

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

Sweet, that sounds good.

Copy link
Member

@RangerMauve RangerMauve 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, thank you!

@makew0rld makew0rld merged commit a266e6b into main May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants