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

Timely communication from_bytes cannot re-use allocations #96

Closed
frankmcsherry opened this issue Sep 7, 2017 · 1 comment
Closed

Timely communication from_bytes cannot re-use allocations #96

frankmcsherry opened this issue Sep 7, 2017 · 1 comment

Comments

@frankmcsherry
Copy link
Member

frankmcsherry commented Sep 7, 2017

Timely communication has a Serialize trait, with methods

pub trait Serialize {
    /// Append the binary representation of `self` to a vector of bytes. The `&mut self` argument
    /// may be mutated, but the second argument should only be appended to.
    fn into_bytes(&mut self, &mut Vec<u8>);
    /// Recover an instance of Self from its binary representation. The `&mut Vec<u8>` argument may
    /// be taken with `mem::replace` if it is needed.
    fn from_bytes(&mut Vec<u8>) -> Self;
}

The first method seems fine for now, but from_bytes has the issue that it is unable to re-use existing Self allocations, as there is nowhere to grab one from (short of a stashed Rc<RefCell<_>> cache).

Timely's Message type gets around this by using Vec<u8> as its backing storage, which is great except that no one else can rely on this, and so returned Message objects just get dropped on the floor, as we don't know how to destructure them into useful parts.

I think we could take inspiration from Rust's Clone trait, which has a method

fn clone_from(&mut self, source: &Self) { ... }

We could totally change the signature of from_bytes to be

fn from_bytes(&mut self, &mut Vec<u8>);

with the intent that one should feel free to re-use as much of &mut self as you can afford, as long as what results is still valid. If you happen to swap around the &mut Vec<u8> in the process, good for you.

This seems strictly better than requiring a freshly constructed Self, as simply clobbering the &mut self input argument is always an option.

@frankmcsherry
Copy link
Member Author

Timely communication has changed substantially, and should no longer have this problem. With #135, RefOrMuts swap method deserializes into an existing allocated object, which addresses this problem.

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

No branches or pull requests

1 participant