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

perf: use msgio pooled buffers for received msgs #500

Merged
merged 1 commit into from
Nov 19, 2022

Conversation

Wondertan
Copy link
Contributor

The issue mentions only receiving buffers, so this PR only changes those. However, there might be value in changing sending buffers as well. Happy to update this in a subsequent commit if needed.

Closes #350

@Wondertan
Copy link
Contributor Author

@vyzo, any interest in this?

@vyzo
Copy link
Collaborator

vyzo commented Sep 20, 2022

ah sorry, didnt notice it.
Can you tag me for review in future prs?

@vyzo vyzo self-requested a review September 20, 2022 06:57
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Thank you for this, it looks like it's going to help quite a bit with memory consumption.

Lets discuss a bit the rpc thingie, and then I am happy to merge.

comm.go Outdated Show resolved Hide resolved
comm.go Outdated Show resolved Hide resolved
@vyzo
Copy link
Collaborator

vyzo commented Sep 22, 2022

@Wondertan ping

@vyzo
Copy link
Collaborator

vyzo commented Sep 22, 2022

Also, lets do send buffers while at it; it can be a separate pr.

@Wondertan
Copy link
Contributor Author

ah sorry, didnt notice it.

nw, someone has to make FVM sound 😉

@Wondertan
Copy link
Contributor Author

ping

@vyzo, done

Also, lets do send buffers while at it; it can be a separate pr.

👌🏻

@Wondertan Wondertan force-pushed the master branch 2 times, most recently from ca3e349 to 137a1ab Compare September 22, 2022 12:07
@Wondertan
Copy link
Contributor Author

Also, updated handlePeerEOF to use msgio

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

need to release the buffer in handleEOF.

comm.go Outdated Show resolved Hide resolved
@vyzo
Copy link
Collaborator

vyzo commented Oct 29, 2022

@Wondertan ping -- lets move this forward?

@Wondertan
Copy link
Contributor Author

Wondertan commented Nov 3, 2022

@vyzo, I am currently on the two-week team onsite, and it's almost the end. Going to bring this over the finish line right after.

The are some unanswered questions in the thread, IIRC

@vyzo
Copy link
Collaborator

vyzo commented Nov 3, 2022

yeah, whenever you can.

I think reverting the eof change for now, or releasing the buffer, will do for merge.

But lets create a follow up issue to discuss the general handling and come up with a good solution that can later be speced for other impls to follow.

@Wondertan
Copy link
Contributor Author

@vyzo, so I reverted the EOF change and started making an issue for it. Now it's only the reading side with pooled buffers. I can also try to squeeze in the write today if we want to.

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

LGTM for now, lets merge and do follow up prs.

@vyzo vyzo merged commit 9c56b2d into libp2p:master Nov 19, 2022
@Wondertan
Copy link
Contributor Author

@vyzo, see #506

yhassanzadeh13 added a commit to yhassanzadeh13/go-libp2p-pubsub that referenced this pull request Nov 24, 2022
* perf: use msgio pooled buffers for received msgs (libp2p#500)

* perf: use pooled buffers for message writes (libp2p#507)

* improve handling of dead peers (libp2p#508)

* chore: ignore signing keys during WithLocalPublication publishing (libp2p#497)

* adds app specific rpc handler

Co-authored-by: Hlib Kanunnikov <[email protected]>
Co-authored-by: Viacheslav <[email protected]>
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.

Reduce memory usage for receive buffers
2 participants