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

multistream-select: Less allocations. #800

Merged
merged 5 commits into from
Jan 9, 2019

Conversation

twittner
Copy link
Contributor

@twittner twittner commented Dec 19, 2018

This PR attempts to avoid creating intermediate Bytes values when sending protocol names, but instead passes them as is to the tokio_codec::Encoder which copies them once into the destination byte buffer.

The dialer_select/listener_select state machines sometimes need to duplicate a single protocol name. This PR chose to require Clone on an item which is the most straightforward and in most cases (slices, Bytes, etc.) the most performant solution. If this is unwanted I can change this PR to perform reiteration to get the protocol name again, but I wanted to get some feedback first as this is a more complicated solution.

@ghost ghost assigned twittner Dec 19, 2018
@ghost ghost added the in progress label Dec 19, 2018
Copy link
Member

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

I would love to remove that Clone in an ideal world, but it's also not the most important thing in my opinion.
You can either do it in this PR, or just merge this and open an issue instead.

protocols: I
},
NextProtocol {
dialer: Dialer<R>,
dialer: Dialer<R, I::Item>,
proto_name: I::Item,
protocols: I
},
SendProtocol {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should mention that this is in fact a Flush. Either change the current confusing name or add a comment.

@ghost ghost assigned tomaka Jan 9, 2019
@tomaka
Copy link
Member

tomaka commented Jan 9, 2019

Tested on Substrate, seems to work just fine.

`SendProtocol` -> `FlushProtocol`
`SendListRequest` -> `FlushListRequest`
@twittner twittner merged commit f195925 into libp2p:master Jan 9, 2019
@twittner twittner deleted the multistream-select-optimisation branch January 9, 2019 14:09
@ghost ghost removed the in progress label Jan 9, 2019
@tomaka
Copy link
Member

tomaka commented Jan 9, 2019

Could you create the issue for removing Clone as well? Since you know more of the context.

@twittner
Copy link
Contributor Author

twittner commented Jan 9, 2019

Done: #836

dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Jan 15, 2019
* upstream/master: (35 commits)
  Bump libp2p-secio and libp2p-core-derive (libp2p#857)
  Fixes to Kademlia queries (libp2p#855)
  Cache the secp256k1 object in secio (libp2p#856)
  Fix custom derive when using a where clause (libp2p#853)
  Bump to 0.2.2 (libp2p#852)
  Bump to v0.2.1 (libp2p#851)
  Add IntoNodeHandler and IntoProtocolsHandler traits (libp2p#848)
  Improve doc aesthetics (libp2p#850)
  Fix compilation of ring (libp2p#846)
  Add some benchmarks for secio (libp2p#847)
  Version 0.2 (libp2p#841)
  Forbid dialing self (libp2p#839)
  Add an Error associated type to transports (libp2p#835)
  Update ring to 0.13 (libp2p#674)
  Move tests to separate files (libp2p#827)
  multistream-select: Less allocations. (libp2p#800)
  Add more methods on Topology (libp2p#826)
  Documentation improve to ping and minor improvements (libp2p#831)
  Floodsub now produces FloodsubEvent (libp2p#823)
  Add RawSwarm::incoming_negotiated (libp2p#821)
  ...
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