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

Fully migrate the whole repo from bytes 0.4 to bytes 0.5 #626

Closed
bstrie opened this issue Feb 6, 2020 · 2 comments · Fixed by #690
Closed

Fully migrate the whole repo from bytes 0.4 to bytes 0.5 #626

bstrie opened this issue Feb 6, 2020 · 2 comments · Fixed by #690

Comments

@bstrie
Copy link
Contributor

bstrie commented Feb 6, 2020

Currently the biggest subtask of #140 is the removal of our duplicated bytes dependency. bytes is a library for zero-copy network protocols. Most of our crates depend on it directly, and all of them depend on it indirectly via its use in interledger-packet, which is the root of our workspace dependencies. interledger-packet uses bytes for zero-copy handling of ILP packets, so it's fundamental to the library. However, when Rust stabilized async/await, the Rust async ecosystem updated to bytes v0.5, which has some big changes compared to bytes v0.4, which is what interledger-rs was originally written to use. As part of our massive async/await migration, v0.5 was used only where necessary and shims were put into place for interoperation with code that used v0.4 but that otherwise didn't need to be touched. These shims are not entirely zero-copy, so to reclaim the original intent of the design of interledger-packet it would be nice to fully migrate to v0.5.

As of this writing here are the libraries that explicitly depend on v0.4:

  • ilp-node
  • btp
  • ccp
  • ildcp
  • packet
  • stream

Here is a non-comprehensive overview of the incompatible changes between v0.4 and v0.5: https://github.com/tokio-rs/bytes/releases/tag/v0.5.0

Here's a WIP PR that ports interledger-packet alone to v0.5 that may be used as a base for a full migration (however, note that it's still not entirely zero-copy; see the TODO comments): https://github.com/interledger-rs/interledger-rs/pull/625/files

@koivunej
Copy link
Collaborator

Been looking out for bytes = "1" update while looking for how to get the codebase up to tokio 1.0. API wise much hasn't changed for bytes but overall there are some comments missing on why do the {Prepare,Fulfill,Reject} refer to BytesMut while having for example the address be Bytes backed. The only reason I can think for this is the mutability aspect of Prepare only called from https://github.com/interledger-rs/interledger-rs/blob/cbdb38763084ff5ab471c25a7de5ed05a409afe9/crates/interledger-service-util/src/exchange_rates_service.rs#L143.

For the serialization there seems to be some duplication between interledger-packet and interledger-btp, possibly other crates as well. I'll try to see if there's a way forward to unify the duplication and turn some parts like "length of length" constant time before any upgrades..

@koivunej
Copy link
Collaborator

I failed to add a comment on #690 but to explain about the BytesMut usage: interledger-packet::packet::{Prepare,Fulfill,Reject} have a BytesMut because Prepare needs to be able to modify the packet representation, and this is expected to happen often. Perhaps it could be done over just Bytes and a method which would allow to use chained bytes. However it's unclear how big of a win this would be. For the long-term storage of Address it might be better to just refer to a standalone Bytes instace instead of part of the incoming packet, though I do not know of any existing places for interning addresses in the codebase.

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 a pull request may close this issue.

2 participants