Skip to content

Unnecessary Box::pin() #201

Open
Open
Unnecessary Box::pin()#201
Labels
P-LowquestionFurther information is requested

Description

@realcr
realcr
opened on May 16, 2019

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?

Activity

  1. pzmarzly commented on May 16, 2019

    @pzmarzly
    pzmarzly
    on May 16, 2019
    Collaborator

    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.

  2. pzmarzly commented on May 16, 2019

    @pzmarzly
    pzmarzly
    on May 16, 2019
    Collaborator

    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.

  3. realcr commented on May 16, 2019

    @realcr
    realcr
    on May 16, 2019
    MemberAuthor

    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?

  4. pzmarzly commented on May 16, 2019

    @pzmarzly
    pzmarzly
    on May 16, 2019
    Collaborator

    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.

  5. pzmarzly commented on May 29, 2019

    @pzmarzly
    pzmarzly
    on May 29, 2019
    Collaborator

    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.

  6. realcr commented on May 30, 2019

    @realcr
    realcr
    on May 30, 2019
    MemberAuthor

    @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.

  7. pzmarzly commented on May 31, 2019

    @pzmarzly
    pzmarzly
    on May 31, 2019
    Collaborator

    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?

  8. realcr commented on Jun 1, 2019

    @realcr
    realcr
    on Jun 1, 2019
    MemberAuthor

    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.

  9. amosonn commented on Jun 1, 2019

    @amosonn
    amosonn
    on Jun 1, 2019 via email
    Collaborator

  10. pzmarzly commented on Jun 1, 2019

    @pzmarzly
    pzmarzly
    on Jun 1, 2019
    Collaborator

    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.

  11. amosonn commented on Jun 1, 2019

    @amosonn
    amosonn
    on Jun 1, 2019 via email
    Collaborator

  12. pzmarzly commented on Jun 27, 2019

    @pzmarzly
    pzmarzly
    on Jun 27, 2019
    Collaborator

    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)

  13. realcr commented on Jun 27, 2019

    @realcr
    realcr
    on Jun 27, 2019
    MemberAuthor

    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.

  14. realcr
    added
    P-Low
    questionFurther information is requested
    on Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P-LowquestionFurther information is requested

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions