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

impl Into<Vec<u8>> for EasyBuf (cf. #120) #162

Merged
merged 2 commits into from
Feb 11, 2017

Conversation

manuel-woelker
Copy link
Contributor

Feedback welcome!

@tailhook
Copy link
Contributor

tailhook commented Feb 7, 2017

Hm, I'm not sure if project owners will require this, but I'd prefer this method be non-allocating if possible (i.e. if underlying buffer is not referenced anywhere). You should be able to achieve that using Arc::make_mut and std::mem::replace.

@tailhook
Copy link
Contributor

tailhook commented Feb 7, 2017

Hm, on the other hand it also requires checking start and end, so maybe it's not worth the complexity.

@manuel-woelker
Copy link
Contributor Author

Yep, that was my reasoning. If performance/memory is an issue, it's best to stick with EasyBuf.

AFAICT, it could only be non-allocating only works iff there are no other references AND start == 0 AND end == 0. This might also make performance more unpredictable.

@alexcrichton
Copy link
Contributor

Perhaps this could be implemented via get_mut which gives mutable access to a Vec<u8>?

@manuel-woelker
Copy link
Contributor Author

manuel-woelker commented Feb 8, 2017

Thanks for the feedback and pointers.
I tried get_mut and Arc::make_mut but that didn't seem to work because I could only get references (&Vec's).
I ended up using Arc::try_unwrap() which seemed like a good fit.

I added assertions to verify if the code is indeed allocating/non-allocating. I would appreciate an evaluation if comparing the pointers is safe/correct/good practice in this context.

@tailhook
Copy link
Contributor

tailhook commented Feb 8, 2017

@manuel-woelker even if it isn't useful here for some reason, you should learn the repace hack or it might be easier to get the idea by looking at swap. I.e. replacing (or swapping) a vector with an empty one is cheap and can be used to move the vector off the mutable pointer.

@alexcrichton
Copy link
Contributor

@manuel-woelker oh I think somethingn like this would work:

mem::replace(&mut *self.get_mut(), Vec::new())

@manuel-woelker
Copy link
Contributor Author

@tailhook @alexcrichton Thanks for the neat trick! So when rustc says "cannot move out of borrowed content", you actually can, as long as you "put something back".

I had to truncate the Vec to length, since get_mut() doesn't truncate it, as it maybe should.
I don't know if that is by design or not. It leads to different results depending on whether the EasyBuf is shared or not.

Compare:

    #[test]
    fn easybuf_get_mut_sliced() {
        let vec: Vec<u8> = (0u8..10u8).collect();
        let mut buf: EasyBuf = vec.into();
        buf.split_off(3);
        println!("Length: {}", buf.len()); // Length: 3
        {
            buf.get_mut(); // Not doing anything with this
        }
        println!("Length: {}", buf.len()); // Length: 10 !!!
    }

    #[test]
    fn easybuf_get_mut_sliced_allocating() {
        let vec: Vec<u8> = (0u8..10u8).collect();
        let mut buf: EasyBuf = vec.into();
        buf.split_off(3);
        // Clone to make shared
        let clone = buf.clone();
        println!("Shared Length: {}", buf.len()); // Shared Length: 3
        {
            buf.get_mut(); // Not doing anything with this
        }
        println!("Shared Length: {}", buf.len()); // Shared Length: 3
    }

@alexcrichton
Copy link
Contributor

@manuel-woelker oh I think you've discovered a bug in EasyBuf!

The snippet I believe should work, but a bug today means it doesn't. For example this fails today (but it should pass)

let vec: Vec<u8> = (0u8..10u8).collect();      
let mut buf: EasyBuf = vec.into();             
buf.split_off(9);                              
buf.drain_to(3);                               
assert_eq!(*buf.get_mut(), [3, 4, 5, 6, 7, 8]);

(adapted from your own test case)

I believe, though, that this can be fixed with a call to buf.drain(self.end..) here perhaps?

Once that's fixed I think you should be able to use the replace construction with no extra calls in the into implementation.

manuel-woelker added a commit to manuel-woelker/tokio-core that referenced this pull request Feb 10, 2017
@manuel-woelker
Copy link
Contributor Author

@alexcrichton Okay, got it working, and added tests for get_mut(). This PR has gotten a bit messy. Do you want me to push a cleaned up version? There is also a TODO here that I could take a stab at in the process.

Also, is there a practical way to make all EasyBuf tests run for both a shared and an exclusive instance?
A macro would probably work but may be overkill.

@alexcrichton
Copy link
Contributor

You could hold onto one of the return values of split_off and drain_to but it may be overkill for this situation. In any case yeah want to squash and I'll merge?

@manuel-woelker
Copy link
Contributor Author

Ok, I rebased and split the PR into the bugfix and the original impl for Into<Vec<u8>>.

Thanks for the mentoring, much appreciated!

@alexcrichton
Copy link
Contributor

Thanks!

@alexcrichton alexcrichton merged commit 574b6f0 into tokio-rs:master Feb 11, 2017
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.

3 participants