Unnecessary Box::pin()

[realcr]

Some places in the code base use Box::pin(...) only to create a future that satisfies Pin requirements. One example from components/channeler/src/connect_pool.rs:

async fn conn_attempt<RA, C, ET>(
    friend_public_key: PublicKey,
    address: RA,
    mut client_connector: C,
    mut encrypt_transform: ET,
    canceler: oneshot::Receiver<()>,
) -> Option<RawConn>
where
    RA: Eq,
    C: FutTransform<Input = (RA, PublicKey), Output = Option<RawConn>> + Clone,
    ET: FutTransform<Input = (PublicKey, RawConn), Output = Option<RawConn>> + Clone,
{
    // TODO: How to remove this Box::pin?
    let connect_fut = Box::pin(async move {
        let raw_conn = await!(client_connector.transform((address, friend_public_key.clone())))?;
        await!(encrypt_transform.transform((friend_public_key.clone(), raw_conn)))
    });

    // We either finish connecting, or got canceled in the middle:
    select! {
        connect_fut = connect_fut.fuse() => connect_fut,
        _ = canceler.fuse() => None,
    }
}

Box::pin(...) is a runtime tradeoff we make only to satisfy the compiler. Is there a way to write this code without using Box::pin(...), and also without using unsafe tricks? It seems to me that we should be able to pin the future to the stack somehow, but I'm not sure if we are supposed to do this.

This is not high priority, but I was curious about this problem for a while. There are other places in the code that have the same problem, you can probably find them by searching for "Box::pin". (Note that there are some legitimate uses of Box::pin around the code too).

@pzmarzly : Do you think you can check this out?

[pzmarzly]

TL;DR async { ... } bad, (...).then(...).map(...) good

I have to admit I'm not too fond of the Pin API, seeing how it semi-magically autoderives Unpin for some futures and not for others.

At first, this reminded me of an issue I had once (How to construct a struct with internal lifetime dependency?). Basically, values on stack move more than I thought they do. In case of conn_attempt function, it looks like connect_fut is local variable, but afaik it is not, as conn_attempt is async, so it returns a future that contains connect_fut in it (similarly to how Iterator-related types are chained). Thus, it looks like all futures need to be pinned at some point, or be Unpin. async { ... } blocks create new future that doesn't have any "anchor", and here it doesn't autoderive Unpin. Getting rid of it helps to save 1 Box::pin invocation: c593a97 . The only reason Box::pin is in error arm is because arms need to have same return types, so I'd need to remove Pin<...> in both arms at the same time, which doesn't sound as impossible as before.

While I was playing with the code, I unrolled select! macro in d194087 . May be helpful if better compiler errors are required at some point while solving this issue.

I agree this is not high priority, in benchmark I quickly made Box::pin(vec![ /* 64 numbers here */ ]) takes 0.028 ms, and that includes creating the Vec.

[pzmarzly]

In the worst case scenario, getting rid of Box::pin would require to rewrite all 97 async {...} blocks. It would decrease code readability for unnoticeable performance improvement. Unless some better way of getting rid of Box::pin is found, method from c593a97 is not worth to pursue.

[realcr]

I have to admit I'm not too fond of the Pin API, seeing how it semi-magically autoderives Unpin for some futures and not for others.

From my understanding Unpin is not derived for generators because they have self references, therefore they can not be moved safely.

In the worst case scenario, getting rid of Box::pin would require to rewrite all 97 async {...} blocks.

I actually want to keep the async blocks. I think that they are a major readability and ergonomics improvement (In the old futures version we didn't have those, and it was super difficult to write readable futures code). What I was wondering about is what is the way normal people are expected to use async blocks with things like the select! macro?

My current (poor) understanding of the Pinning thing is that I can work with a structure, but I have to pin it first somewhere. Currently we pin it to the heap using Box::pin. I use Box::pin because it was the only workaround I could find for my compilation problems when the Pin semantics were first introduced.

I'm almost sure there is another way. We just don't know about it yet. Maybe, for example, I can pin the structure to the stack and then work with it?

I was wondering if you could check around to find if there is another way (Besides changing the async blocks). I agree with you that this is not an urgent issue, nor will it give us serious speed-ups. I'm just really curious about what is the right thing to do here. We were promised zero cost abstraction futures, but this is a strange case where I don't know how to do it.

One place where I thought we might find an answer would be the sources of futures?

[pzmarzly]

Well, futures are now split between the compiler, std and futures-rs, so investigating it may take some effort. Before doing that, I'll try to search some more and try asking some people.

[pzmarzly]

Okay, here's another theory of mine.

In this presentation (from ~14:20 to ~16:00), WithoutBoats stated the goal is to have "one heap allocation per future".

After seeing this commit, I wouldn't be surprised if it turned out every async fn makes a Box, which would mean heap is used all over the place in futures ecosystem and it's perfectly acceptable to allocate a lot. BTW that commit was linked in this Reddit thread today.

But then, why is it possible to avoid Boxing with then()? I think it's not the data that need to be Pinned/Boxed, but the async code/closure. Then the reason why futures based on then() and map() don't need a Box is that they are a separate type, so their logic is supplied by impl Future for T blocks.

Then again, I'm not really convinced.

[realcr]

@pzmarzly: Thanks for the work you did to figure this issue out.

one heap allocation per future

I could be wrong, but let me try to write what I understand about those topics.
First, about the "one heap allocation per future", what I think withoutboats means is that there is only one allocation for every future, and it happens the moment you put it into an executor (For example, using spawn).

This means that you only allocate the "top level" future. If you have inside your async func some more async calls and more futures you construct, those futures are not allocated. All the futures together turn into a very large state machine that eventually allocated only once when put into an executor.

I wouldn't be surprised if it turned out every async fn makes a Box

I think that async {} blocks and async functions do not create Boxes by default. async is a very low level primitive in Rust, it will probably not be the "Rust way" to have implicit allocations for such a low level primitive.

One strong evidence that I have for this opinion this issue that we opened for offst:
rust-lang/futures-rs#1330
Where a runtime stack overflow occurs due to many nested async blocks.
I see this as evidence that async blocks live on the stack, and not automatically allocated on the heap.

A hint from futures-rs

There is some other interesting thing I noticed lately that might help with solving this issue.
I have seen this piece of code in the futures-rs codebase (futures-test/src/future/pending_once.rs):

// ...
use std::pin::Pin;
use pin_utils::{unsafe_pinned, unsafe_unpinned};

/// Combinator that guarantees one [`Poll::Pending`] before polling its inner
/// future.
///
/// This is created by the
/// [`FutureTestExt::pending_once`](super::FutureTestExt::pending_once)
/// method.
#[derive(Debug, Clone)]
#[must_use = "futures do nothing unless you `.await` or poll them"]
pub struct PendingOnce<Fut: Future> {
    future: Fut,
    polled_before: bool,
}

impl<Fut: Future> PendingOnce<Fut> {
    unsafe_pinned!(future: Fut);
    unsafe_unpinned!(polled_before: bool);

    pub(super) fn new(future: Fut) -> Self {
        Self {
            future,
            polled_before: false,
        }
    }
}

Note the unsafe_pinned! and unsafe_unpinned! macros. I'm not really sure how they work, but they can be traced back to the pin_utils crate. Checking the documentation led me to this page:
https://docs.rs/pin-utils/0.1.0-alpha.4/pin_utils/macro.pin_mut.html
which contains this example:

let foo = Foo { /* ... */ };
pin_mut!(foo);
let _: Pin<&mut Foo> = foo;

Maybe we can do something like this to pin things to the stack?
I'm not fully sure how it all works, I'm also not sure if pin_mut is a macro that is always safe to use. (At least it doesn't have the unsafe word inside of its name).

Other directions

Some things that I think we can try:

• Experiment with pin_mut and try to use it instead of Box::pin() to make the first code example in this issue work
• Find other usages of pin_utils in futures-rs and see how it works.
• Open an issue at futures-rs, asking what is the right way of pinning things while still having zero cost abstraction? I think that if we do this, it is a good idea to introduce our use case: A simplified example of the code above in which we have to use Box::pin() to get a compiling version. (If you are shy to do this, I will be happy to do it myself (: ). I am pretty sure that if we don't fully understand this, there are probably other people that have the same problem.

[pzmarzly]

pin_mut code looks so simple that I'd be surprised if that's the answer. Though, that would be a pleasant surprise.

I tried it on pzmarzly/unsafe-pin, it compiles, but I haven't tested whether it panics when connecting to relay/index. Could you check that?

[realcr]

I tried it on pzmarzly/unsafe-pin, it compiles

Cool!

pin_mut code looks so simple that I'd be surprised if that's the answer.

I have to agree, it feels too good to be true. What I fear is that we are doing something unsound here, but we can not see it or experience it in our tests.

I tried it on pzmarzly/unsafe-pin, it compiles, but I haven't tested whether it panics when connecting to relay/index.

If you are working from master, running cargo test should run all the integration tests, which also checks connection to relay/index. If the tests pass, it means that it probably doesn't crash. (Unfortunately, we can't be sure that it is actually safe).

Note: In the new "atomic payments" design (PR #199) the integration tests are not yet working, so it's not very useful to test it there.

One thing I think we can do from here is write a simplified self contained example of the problem code with Box::pin(), then write a simplified self contained code with the pin_mut solution. Finally we can open an issue at the futures github repository with the two self contained examples and ask if we are doing the right thing.

[amosonn]

There's a similar (but slightly different-looking) version of this in the RFC itself; maybe it's better to use that to be safe: https://github.com/rust-lang/rfcs/blob/master/text/2349-pin.md#stack-pinning-api-potential-future-extension

…

On Sat, Jun 1, 2019, 12:26 real ***@***.***> wrote: I tried it on pzmarzly/unsafe-pin, it compiles Cool! pin_mut code looks so simple that I'd be surprised if that's the answer. I have to agree, it feels too good to be true. What I fear is that we are doing something unsound here, but we can not see it or experience it in our tests. I tried it on pzmarzly/unsafe-pin, it compiles, but I haven't tested whether it panics when connecting to relay/index. If you are working from master, running cargo test should run all the integration tests, which also checks connection to relay/index. If the tests pass, it means that it probably doesn't crash. (Unfortunately, we can't be sure that it is actually safe). Note: In the new "atomic payments" design (PR #199 <#199>) the integration tests are not yet working, so it's not very useful to test it there. One thing I think we can do from here is write a simplified self contained example of the problem code with Box::pin(), then write a simplified self contained code with the pin_mut solution. Finally we can open an issue at the futures github repository with the two self contained examples and ask if we are doing the right thing. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#201>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAX7HLNRHCNJ3ASTI7Q6PVTPYI6GBANCNFSM4HNJ5ANA> .

[pzmarzly]

If we go this way, I'd prefer to use pin_mut macro over RFC example implementation. I find it more elegant and error-prone:

let f = async { ... };
pin_mut!(f);
select! { f => ..., ... };

vs

let f = async { ... };
let mut f = pinned(f);
let f = f.into_pin();
select! { f => ..., ... };

We could just copy that macro, though MIT license would require us to attribute its author.

BTW in pzmarzly/unsafe-pin I was modifying master, so this solution seems to be okay.

[amosonn]

It seems there might be some issue with the drop guarantees: https://doc.rust-lang.org/std/pin/index.html#drop-guarantee which make the RFC version unsound. The problem is, that if you just pin something and then "go on", nobody guarantees it'll be dropped (and not forgotten), and once you return from the function the memory (on stack) is reused. I'm not sure whether the macro version is sound. A different, less ergonomic stack-pinning API is suggested here: rust-lang/rfcs#2349 (comment) A longer, somewhat relevant post is here (only started reading): https://www.ralfj.de/blog/2018/04/10/safe-intrusive-collections-with-pinning.html

…

On Sat, Jun 1, 2019, 18:17 Paweł Zmarzły ***@***.***> wrote: If we go this way, I'd prefer to use pin_mut macro over RFC example implementation. I find it more elegant and error-prone: let f = async { ... };pin_mut!(f);select! { f => ..., ... }; vs let f = async { ... };let mut f = pinned(f);let f = f.into_pin();select! { f => ..., ... }; We could just copy that macro, though MIT license would require us to attribute its author. BTW in pzmarzly/unsafe-pin I was modifying master, so this solution seems to be okay. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#201>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAX7HLOTYIFXLVMLVM2MRODPYKHHNANCNFSM4HNJ5ANA> .

[pzmarzly]

The Async Book has chapter on pinning, which gives pin_mut! a green light. But I guess I should finally ask someone more experienced about it (and even then, check if putting stack pinning everywhere wouldn't overflow the stack due to offst complexity)

[realcr]

For some reason I didn't notice that conversation in this topic, I have no idea why. Sorry about that!

I am also aware of some unsoundness issues with the stack pinning. I'm not yet sure what is the right direction here. I think we should leave this open and keep using Box::pin(...) for the time being, until we know for sure what is the correct and sound direction for stack pinning.