"impl Trait" in stable Rust just arrived #105

swizard0 · 2018-05-12T10:02:11Z

impl Trait has finally out in recent stable 1.26, so we could get rid of all Box-es in our code.

In theory this means less alloc/free, no vtable method calls, aggressive compiler inlining and optimization abilities, etc :)

Keruspe · 2018-05-12T10:04:27Z

I want to issue a 0.12.0 soon-ish before doing that, but thanks for the PR! Could you rebase on top of current master please?

…

On Sat 12 May 2018 at 12:02, swizard0 ***@***.***> wrote: impl Trait <https://blog.rust-lang.org/2018/05/10/Rust-1.26.html> has finally out in recent stable 1.26, so we could get rid of all Box-es in our code. In theory this means less alloc/free, no vtable method calls, aggressive compiler inlining and optimization abilities, etc :) ------------------------------ You can view, comment on, or merge this pull request online at: #105 Commit Summary - implement `Channel::basic_qos` - Merge remote-tracking branch 'upstream/master' - upstream merge smallfix - get rid of all `Box` - do not clone by hand - less strict tokio semver - tests will not work with `tokio 0.1.1` - make the same version as in upstream File Changes - *M* futures/Cargo.toml <https://github.com/sozu-proxy/lapin/pull/105/files#diff-0> (2) - *M* futures/src/channel.rs <https://github.com/sozu-proxy/lapin/pull/105/files#diff-1> (257) - *M* futures/src/client.rs <https://github.com/sozu-proxy/lapin/pull/105/files#diff-2> (102) - *M* futures/src/transport.rs <https://github.com/sozu-proxy/lapin/pull/105/files#diff-3> (12) Patch Links: - https://github.com/sozu-proxy/lapin/pull/105.patch - https://github.com/sozu-proxy/lapin/pull/105.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#105>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANm3h99QSQiOUk_z5_GTfC3IEwCLSxnks5txrMjgaJpZM4T8Wou> .

Keruspe

Please rebase against current master once more, and this time try not to revert all the recent changes as part of your commit...

Keruspe · 2018-05-15T20:08:31Z

futures/src/channel.rs

+    pub fn create(transport: Arc<Mutex<AMQPTransport<T>>>) -> impl Future<Item = Self, Error = io::Error> + Send {
        let channel_transport = transport.clone();
        let create_channel = future::poll_fn(move || {
-            let mut transport = try_lock_transport!(channel_transport);


why are you reverting that?

It's probably an automatic merge issue, sorry about it. I'll rebase against master once more.

It's not the only place where you reverted stuff btw, especially this pattern.
And please amend the fixes to your commit

Could you please point me the other places? I couldn't find them reading diff.

It appears you fixed it since the review

Keruspe · 2018-05-15T20:09:10Z

futures/src/channel.rs

-    pub fn access_request(&self, realm: &str, options: &AccessRequestOptions) -> Box<Future<Item = (), Error = io::Error> + Send + 'static> {
-        self.run_on_locked_transport("access_request", "Could not request access", |transport| {
-            transport.conn.access_request(self.id, realm.to_string(),
+    pub fn access_request(&self, realm: &str, options: &AccessRequestOptions) -> impl Future<Item = (), Error = io::Error> + Send {


Why the move and all the cloning everywhere which wasn't needed before?

The other my option for impl Trait result was the following signature:

pub fn access_request<'a>(&self, realm: &str, options: &'a AccessRequestOptions) -> impl Future<Item = (), Error = io::Error> + Send + 'a {

Here I decided to just clone options parameter and put less restrictions for return type. The cloning should be cheap and anyways there is .to_string() invocation which is more expensive because of allocation.

But why is that needed now and wasn't before? Because of the FnOnce change?

Keruspe · 2018-05-15T20:10:01Z

futures/src/channel.rs

-    fn run_on_locked_transport_full<Action, Finished>(&self, method: &str, error: &str, action: Action, finished: Finished, payload: Option<(u16, &[u8], BasicProperties)>) -> Box<Future<Item = Option<bool>, Error = io::Error> + Send + 'static>
-        where Action:   Fn(&mut AMQPTransport<T>) -> Result<Option<RequestId>, lapin_async::error::Error>,
+    fn run_on_locked_transport_full<Action, Finished>(&self, method: &str, error: &str, action: Action, finished: Finished, payload: Option<(u16, &[u8], BasicProperties)>) -> impl Future<Item = Option<bool>, Error = io::Error> + Send
+        where Action:   FnOnce(&mut AMQPTransport<T>) -> Result<Option<RequestId>, lapin_async::error::Error>,


Why this FnOnce change?

Because it doesn't compile otherwise. If you believe that there surely must be a Fn, you could try to figure out the signature yourself (I failed personally).

What did the compilation error look like with Fn?
It seems weird to me that impl trait causes such a regression, and if it does, I'd prefer not to use it there.

Keruspe · 2018-05-15T20:10:46Z

futures/src/channel.rs

        if let Ok(mut transport) = self.transport.lock() {
            match action(&mut transport) {
-                Err(e)         => Box::new(future::err(Error::new(ErrorKind::Other, format!("{}: {:?}", error, e)))),
+                Err(e)         => Either::A(future::err(Error::new(ErrorKind::Other, format!("{}: {:?}", error, e)))),


Why all the Either?

Because here there are two different types to return: futures::FutureResult and futures::PollFn. One has to either Box them (for virtual dispatching) or return an futures alternative (either::Either)

Keruspe · 2018-05-15T20:11:25Z

futures/src/client.rs

+    pulse: PF,
 }

-impl Heartbeat {


Why are you reverting this too?

That was not a revert, it's rather a refactoring. It is my attempt to avoid Box-ing the pulse field.

If you can find out a better solution that would be great.

I'd prefer to leave it as is actually

swizard0 · 2018-05-16T09:32:01Z

Okay it's starting to look like I'm trying to persuade you to accept these patches which you really don't like :)

Please feel free to reject this pull request completely, it is your own project after all.

I've already tested and put the library fork into production with all these impl Trait changes so I would really rather not revert them.

Keruspe · 2018-05-16T09:37:16Z

My basic point is:

please read your commits before submitting to ensure you really intended to submit all the changes they contain (e.g. the various reverts that were not intended shouldn't have been submitted)
while impl trait is clearly an improvement overall, it doesn't mean that every use of Box has to be converted, sometimes dynamic dispatch can be the right call, especially if it makes the code simpler.

I haven't given a try at impl trait yet, but regarding the FnOnce, I wonder what the compilation error was with Fn, because the implicaitons of FnOnce actually adds more allocations than it removes because of all the clones.
Regarding the heartbeat, if you look at the heartbeat rework PR, your versions looks a lot like a previous iteration of the code, and it was moved to be this way as it's way clearer, that's why I thought you were doing another unintended revert there

swizard0 · 2018-05-16T10:16:42Z

FnOnce actually adds more allocations than it removes because of all the clones.

If you look carefully the relevant changes you'll see that there is only one new clone — for options argument. The rest were already there before.
Cloning options does not involve allocations because of only integral types inside (I believe we could even #[derive(Copy)] for it)

Regarding the heartbeat ... it was moved to be this way as it's way clearer

It's definitely clearer. I moved the future assembling there just because there is no way to choose parameter PF for Heartbeat<PF> from inside it's own constructor, so it has to be done externally.

Maybe there is a way to still avoid Box but make the code a bit clearer. I could try to figure out something if you want.

Keruspe · 2018-05-16T12:14:02Z

I'll give it further thoughts.
I'd prefer to keep the Heartbeat stuff as-is for now though, until we find some less intrusive workaround

Keruspe · 2018-05-18T13:38:08Z

Could you resubmit without the heartbeat part?

swizard0 · 2018-05-18T14:02:17Z

What about this solution for Heartbeat?

It's mostly the same as with regular constructor but the code is in separate function instead.

Keruspe · 2018-05-18T14:28:09Z

That could be better.
I'd prefer to get the rest in first to get a better point of view after playing a little with it, and have the heartbeat part in a separate pull request afterwards

Keruspe · 2018-06-03T12:51:51Z

Stuff in this area has evolved to fix some issues and prepare for 0.12 which will be released real soon now.
Could you rebase your work and split the heartbeat commit from the rest please?

swizard0 · 2018-06-04T14:28:26Z

okay i'll try to

Keruspe

Mostly LGTM apart from the few comments.
Will merge as soon as those are addressed and 0.12 is out

Keruspe · 2018-06-04T14:47:38Z

futures/src/channel.rs

 impl<T: AsyncRead+AsyncWrite+Send+'static> Channel<T> {
    /// create a channel
-    pub fn create(transport: Arc<Mutex<AMQPTransport<T>>>) -> Box<Future<Item = Self, Error = io::Error> + Send + 'static> {
+    pub fn create(transport: Arc<Mutex<AMQPTransport<T>>>) -> impl Future<Item = Self, Error = io::Error> + Send {


Why are you removing all the 'static lifetimes?

but they are not needed with impl Trait anymore

so I just don't see why leave unnecessary restrictions

They can be needed in some scenarii actually from what I understand, and it doesn't harm to keep those around I'd say.
If we can add some "guarantees" to the return type "for free", I don't see why we should remove those

Are you sure about these "additional guarantees for return type"?

I believe that 'static within type like Box<SomeTrait + 'static> means just "there is no non-static references inside my trait object", isn't it?

And in case of impl Trait compiler already has a full type in its hands so it can easily examine it itself.

I'm not 100% sure, but I really see no harm in keeping those markers in place

I don't know if there is any harm in them, but they are really not needed :) Even if a generic argument requires 'static the compiler could check it itself (proof: https://play.rust-lang.org/?gist=037be4712b2acfc8411b2725e7ae94ae&version=stable&mode=debug). Probably these markers are added automatically but I'm not sure.

It just works in a different way than in trait objects.

I just realized we can remove Send marker as well! Should I do it?

Keruspe · 2018-06-04T14:48:08Z

futures/src/channel.rs

                options.mandatory, options.immediate).map(Some)
        }, move |conn, delivery_tag| {
-            conn.channels.get_mut(&channel_id).and_then(|c| {
+            conn.channels.get_mut(&channel_id).and_then(move |c| {


Is this needed?

not really i believe

Keruspe · 2018-06-04T14:49:29Z

futures/src/channel.rs

    }

-    fn run_on_locked_transport_full<Action, Finished>(&self, method: &str, error: &str, action: Action, finished: Finished, payload: Option<(Vec<u8>, BasicProperties)>) -> Box<Future<Item = Option<RequestId>, Error = io::Error> + Send + 'static>
+    fn run_on_locked_transport_full<Action, Finished>(&self, method: &str, error: &str, action: Action, finished: Finished, payload: Option<(Vec<u8>, BasicProperties)>) -> impl Future<Item = Option<RequestId>, Error = io::Error> + Send


Can you leave this one unchanged with the Box? I have a few changes locally which will allow making it impl trait without the Either stuff

Do you want to just remove Either or to leave Box in function signature?

If second, we have to box wait_for_answer result as well

Just leave the Boxing for those two for now then please, it'll go away just after :)

Keruspe · 2018-06-04T14:56:31Z

Sorry for the mess this PR has become, I'm really eager to land this, you just happen to have created it when development was really active to get some bugfix done and I want those bugfixes to be released in a version that does not depend on a compiler from a couple of weeks ago

swizard0 · 2018-06-04T15:51:46Z

Is the current pr okay? I could add heartbeat refactoring here also as a separate commit if you want.

Keruspe · 2018-06-04T21:16:15Z

I prefer to keep heartbeat for later.
The PR is fine as it is now.
I just pushed a few changes that should allow using impl trait without Either in client.rs and in run_on_locked_transport

Wrt the + Send + ‘static : after thinking more about it and about your example => indeed that doesn’t change anything for calling code, but it enforces the restrictions inside the library,. This way if for some reason we have an object somewhere which is !Send, the library will fail to build. If we remove the markers, the library will build and some application using it might no longer build.
I definitely think we should keep those around

swizard0 · 2018-06-04T22:10:46Z

Ok it sounds reasonable. I'm going to return markers and rebase the pr.

coveralls · 2018-06-04T22:51:02Z

Coverage remained the same at 55.562% when pulling 23defb8 on swizard0:pull_req_impl_traits into 264c294 on sozu-proxy:master.

Keruspe · 2018-06-06T09:23:27Z

@swizard0 thanks, btw.

I reworked the heartbeat stuff a bit, not sure if it's worth porting this one to impl trait as it's only 1 box allocation now, and it's only done once per connection so it's quite cheap.
I tried to nonetheless, but hit some compiler error "expected type parameter, found anonymized type" which I'm not sure how to solve.
If you feel like it should be done and wanna give it a try, you can do so, now that everything else is in order

swizard0 force-pushed the pull_req_impl_traits branch from bf018bd to 65ba8cb Compare May 12, 2018 10:44

Keruspe requested changes May 15, 2018

View reviewed changes

swizard0 force-pushed the pull_req_impl_traits branch from 65ba8cb to ec155ee Compare May 15, 2018 22:24

swizard0 force-pushed the pull_req_impl_traits branch 3 times, most recently from 6020bab to e8e7a8a Compare May 18, 2018 16:13

swizard0 force-pushed the pull_req_impl_traits branch from e8e7a8a to 97a7c2d Compare June 4, 2018 14:45

Keruspe requested changes Jun 4, 2018

View reviewed changes

swizard0 force-pushed the pull_req_impl_traits branch from 97a7c2d to 0b96e4a Compare June 4, 2018 15:47

"Box" -> "impl Trait" refactoring

23defb8

swizard0 force-pushed the pull_req_impl_traits branch from 0b96e4a to 23defb8 Compare June 4, 2018 22:41

Keruspe approved these changes Jun 5, 2018

View reviewed changes

Keruspe merged commit 81ad3dd into amqp-rs:master Jun 5, 2018

Uh oh!

"impl Trait" in stable Rust just arrived #105

"impl Trait" in stable Rust just arrived #105

Uh oh!

Conversation

swizard0 commented May 12, 2018

Uh oh!

Keruspe commented May 12, 2018 via email

Uh oh!

Keruspe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swizard0 commented May 16, 2018

Uh oh!

Keruspe commented May 16, 2018

Uh oh!

swizard0 commented May 16, 2018

Uh oh!

Keruspe commented May 16, 2018

Uh oh!

Keruspe commented May 18, 2018

Uh oh!

swizard0 commented May 18, 2018

Uh oh!

Keruspe commented May 18, 2018

Uh oh!

Keruspe commented Jun 3, 2018

Uh oh!

swizard0 commented Jun 4, 2018

Uh oh!

Keruspe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment