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

Unused arguments to async fn are dropped too early #54716

Closed
Nemo157 opened this issue Oct 1, 2018 · 25 comments
Closed

Unused arguments to async fn are dropped too early #54716

Nemo157 opened this issue Oct 1, 2018 · 25 comments

Comments

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Oct 1, 2018

Unused arguments to async fn are not moved into the resulting generator so are dropped before the future runs, here's some example psuedo-code demonstrating this (full running playground example here):

async fn foo(log1: DropLog, _log2: DropLog) {
    println!("Got log1: {:?}", log1);
}

fn bar(log1: DropLog, _log2: DropLog) {
    println!("Got log1: {:?}", log1);
}

fn main() {
    foo(DropLog(1), DropLog(2)).poll(...);
    println!();
    bar(DropLog(3), DropLog(4));
}

which gives the output:

Dropped DropLog(2)
Got log1: DropLog(1)
Dropped DropLog(1)

Got log1: DropLog(3)
Dropped DropLog(4)
Dropped DropLog(3)

I found this because of a related issue with futures-await(0.1) posted to urlo, it's at the very least surprising behaviour that needs documenting if not a bug.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Mar 5, 2019

Marking this as blocking stabilization of async-await. Obviously, changing the semantics here would alter the semantics of stable code. Moreover, there's a good case to be made that the current semantics are incorrect. @cramertj and I (along with a few others) discussed this in a recent meeting. Our conclusion was that we effectively wanted a desugaring like the following:

async fn foo(<pattern> @ x: Type) {
}

// becomes
fn foo(<pattern> @ x: Type) {
  async move {
    let <pattern> = x;
  } // <-- as you "exit" the async block
}

where x is a kind of "fresh name" not visible to the user.

Some of the key test cases we want to consider would be:

  • async fn foo(_: Foo) -- the Foo value should be dropped after the future is "waited"
  • async fn foo(_x: Foo) -- same

This is quite analogous then to the behavior for fn foo(_: Foo) and friends -- basically after the "return" executes, the parameters are dropped (whereas current behavior drops before the body of the function even begins (i.e., before the future is polled)).

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Mar 5, 2019

Tagging as E-needs-mentor as we need to write up some notes on how to alter the desugaring here. One bit I can see being complex is that the HIR presently has no way to "name" a parameter apart from entering a pattern.

@cramertj
Copy link
Member

@cramertj cramertj commented Apr 2, 2019

(nominating for discussion in T-lang to resolve the desired behavior)

@cramertj
Copy link
Member

@cramertj cramertj commented Apr 2, 2019

Summary of the current issues by @davidtwco can be found on Zulip.

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Apr 4, 2019

blocks #50547

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Apr 4, 2019

At the T-lang meeting, @cramertj floated the idea that dropping these unused parameters early could be regarded as a useful feature for async fn and we should give it consideration as such.

The main motivation given, as I understand it, is that each keeping such parameters alive (for their later dropping) adds to the space overhead of Futures, and may be impeding other optimizations downstream.


My reaction to the idea of treating the current behavior as "the specification" rather than "a bug" is as follows:

I am a bit nervous about trying to add a feature (async fn in this case) that leverages the existing syntax as a way to say "this new thing (async fn) is analogous to something you already know (fn)" while at the same time throwing in exceptions to the analogy that are not immediately motivated by the nature of the feature itself.

I do understand the idea of "there is expense associated with these droppable parameters, in terms of the Future having to carry them around. We can avoid that expense by dropping them early." My question is whether that optimization (which we should be able to apply to non-droppable parameters with no semantically observable effect) outweighs the conceptual cost for people trying to use the feature.

I suggested in the meeting that I would be more amenable to adding some new feature that allows one to specify this intention explicitly for a parameter. E.g. an attribute like async fn foo(p1: Droppy, #[drop_early] _p2: Droppy).

  • Then calling foo(actual1, actual2) would cause actual2 to be dropped before we even enter the body of foo itself.
  • One can readily imagine supporting the #[drop_early] attribute in non-async fn as well; this would thus strengthen the ability to reason via analogy, rather than keeping exceptional cases in mind.
  • There was a counter-argument stated in the meeting that such an approach would lead to people not knowing about the attribute, or blindly adding it everywhere.
  • But I still think there is a benefit to keeping the semantics of fn and async fn as close as we can, where possible, and then allowing the explicit expression of exceptions like this where the developer deems it necessary.
@Centril
Copy link
Contributor

@Centril Centril commented Apr 4, 2019

We discussed this on this week's lang team meeting but I think we didn't reach any consensus.

Here are some of my own thoughts:

  • An important justification for async fn is that they feel like synchronous functions, so it is valuable if this is adhered to where reasonable. I think changing the drop order can be surprising. In particular, I believe @withoutboats noted (iirc, may be wrong) that you might call a trait function expecting a certain drop order from the function. However, because the function's internals (in the impl block) was structured in a certain way this resulted in different semantics.

    • I believe this could be a semver hazard.
    • My overall feeling is similar to @pnkfelix here re. "while at the same time throwing in exceptions to the analogy"
  • In some cases you can change the order through using async { ... } blocks. @cramertj noted that this won't work for traits.

  • Unused arguments feel rather niche and async { ... } blocks taking care of inherent and free async functions leaves async trait methods which makes it more niche.

  • The performance benefits are not clear at the moment other than to say that the generator's size may grow (but those are not numbers one can reason about on a larger scale).

  • Re. #[drop_early], my main concern is that such knobs further adds to complexity and we should approach that cautiously. There's a benefit in being able to ship such an attribute later if there's substantial evidence that it is necessary.

@cramertj
Copy link
Member

@cramertj cramertj commented Apr 4, 2019

@pnkfelix

But I still think there is a benefit to keeping the semantics of fn and async fn as close as we can, where possible, and then allowing the explicit expression of exceptions like this where the developer deems it necessary.

I think there's great value in matching the expectation that developers have. However, I don't think that many developers have an expectation about the order in which unused arguments are dropped.
As far as an opt-in is concerned, I imagine this being a "death by a thousand cuts" style problem where developers see that their overall memory usage is too high as a result of a large number of potential optimizations like this one that may have been missed-- I don't believe they will know or even think to look for a flag which allows them to turn on early drop; instead, they will conclude that Rust hasn't managed to meet their memory usage expectations and be frustrated, likely handing some more money over to AWS or whomever in order to get some extra RAM without ever hearing about the potential opportunity to drop parameters early (or this, that, and some other thing).

In the end, I think that most Rust developers don't have an intuition about when unused parameters are dropped, and the cases where the effects would be observable are a much smaller set of circumstances. If and when developers do notice and learn about it (which I think will be rare) they have simple and obvious workarounds (manually calling drop), compared to the relatively complicated and undiscoverable workarounds for "too much memory usage."

@Nemo157
Copy link
Member Author

@Nemo157 Nemo157 commented Apr 4, 2019

If and when developers do notice and learn about it (which I think will be rare) they have simple and obvious workarounds (manually calling drop)

I think a key here is that if this differs from normal functions the time between noticing an effect from it and learning about why and how to workaround it must be kept as short as possible. The only reason I even learnt about this issue was because of the linked thread on urlo from a user of the futures-await macro, so even with the very small number of current users this is being noticed in the wild.

@cramertj
Copy link
Member

@cramertj cramertj commented Apr 5, 2019

Hm, looking at that code that actually seems like the behavior I would expect-- trying to receive from a channel that will never have any messages sent to it again should result in a None result, and the fact that the None was delivered sooner than would happen if we matched the semantics elsewhere seems like a good thing-- no tasks were unnecessarily suspended etc. while waiting for a signal of completion from the channel. This means that the current behavior gives not only a size optimization, but also a runtime performance improvement.

However, it is certainly useful feedback to know that people have noticed this in practice, and it's helpful to know that it was confusing "in the wild."

I'm pretty split in my thoughts on this.

@najamelan
Copy link
Contributor

@najamelan najamelan commented Apr 6, 2019

In the code from the user forum, it seems to me there is a logic error. The user wants to select on 2 channels, detecting which one closes first, while expecting them to close both at the same time (when the fn having the senders ends). This doesn't make sense, so I don't think it's rusts fault if this breaks. In an ideal world there would be a compiler warning or lint detecting logic errors, but that might not land before another 15y I suppose.

As a user I expect rust to generate the most performant code. If I'm not using something, it doesn't need to stay around. Channels seem like a special case here where dropping has side effects elsewhere. That seems to be a rather niche situation to me. More niche than having parameters forced onto you when implementing traits. It seems reasonable to demand that people using drop for side effects must be aware implementation details like early dropping of unused params.

I wonder why unused parameters are not dropped early in sync fn. Would it not also allow optimization there? Could we test if it would break any existing code if drop early was consistent behaviour all over? Since it's easy to opt out (manual drop), and gives most performant result by default, this seems like the ideal solution?

I see the following possibilities:

  1. consistent drop early everywhere with opt out: manual drop(sb)
  2. drop early in generator only with opt out: manual drop(sb)
  3. opt in: #[drop_early]
  4. never drop early (for now? with possibility of (1) at some point in the future?)

I think this list is ordered in terms of advantages, but the disadvantages differ:

    • not researched
    • not implemented
    • breaking change
    • inconsistency between async and sync fn
    • potential bad surprise for users
    • does sync fn misses out on certain optimizations?
    • adding another boilerplate language attribute
    • people don't benefit by default
    • people knowing about this will probably add it on all unused parameters by default reverting to the bad surprises of (2) when things break
    • everybody pays extra cost always

Personally (4) would make me kind of sad. (1) seems interesting but probably is a lot of work and it might be to much of a breaking change, which we probably won't know before doing some serious testing? (2) and (3) are compromises.

Question, do we pay unnecessary costs right now for sync fn? Do we allocate space on the stack or in registers for parameters that are never used? If so, option (2) also makes me kind of sad...

@Nemo157
Copy link
Member Author

@Nemo157 Nemo157 commented Apr 8, 2019

I wonder why unused parameters are not dropped early in sync fn. Would it not also allow optimization there? Could we test if it would break any existing code if drop early was consistent behaviour all over?

Stack values that are bound to a variable are dropped at the end of scope. If function arguments were guaranteed the same drop order as local variables you could technically take in a MutexGuard as a parameter and assume that it is locked until the end of your function without once using it.

The unintuitive thing is that not binding an argument (fn foo(_: DropLog)) still only drops that argument at the end of the scope, I would have expected this to do a similar thing as the proposed #[drop_early] attribute.

Is argument drop order actually defined anywhere currently? From the issues noticed on the linked PR it seems like it's not the same as local variables.


you might call a trait function expecting a certain drop order from the function. However, because the function's internals (in the impl block) was structured in a certain way this resulted in different semantics.

As far as I'm aware a trait function's contract doesn't include the drop order of its arguments, so this seems like an implementation detail that can't be relied on anyway.


In some cases you can change the order through using async { ... } blocks. @cramertj noted that this won't work for traits.

More niche than having parameters forced onto you when implementing traits.

This appears to be based on a version of "async fn in traits" where you cannot implement the function via some form of -> impl Future + async { ... }, if the latter were allowed then you can just not move the parameter into the generated future and it will be dropped during the synchronous setup of the future. Supporting this is important for more performance reasons than just dropping unused arguments, you might be able to synchronously pre-calculate a smaller value from some of the arguments to reduce the data required to be stored in the future.

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Apr 10, 2019

From the issues noticed on the linked PR it seems like it's not the same as local variables.

I think the rules for formal parameters are the same as local variables. (There may be some discrepancies from ReScope implementation artifacts within rustc itself, but I do not think those are relevant to the example on the linked PR.)

In both formal parameters and local bindings, the interaction between _-(sub)patterns, de-structuring bind, and LIFO-vs-FIFO drop orders can yield potentially surprising results.

  • Also, RFC 1857 explicitly did not specify the drop order for closure captures, which means some of the examples on the linked PR might change in the future.

Update: Okay, I realized in a meeting today that the handling of temporary R-values leads to the very case under discussion now, where local let-bindings seem to behave differently than formal parameters:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=592b36f7591802fcdef1e6154b026b58

Namely, if you do let _ = EXPR; BODY, the value computed by EXPR does not live beyond that statement. But if you do (|_| { BODY })(EXPR) (and by analogy, the similar variant with fn), the value computed by EXPR will live beyond the execution of BODY.

  • This is a consequence of Rvalue lifetime rules.
  • Note in particular how the linked playpen shows the behavior of fn vs let vs match vs closures.
@comex
Copy link
Contributor

@comex comex commented Apr 18, 2019

Some code, especially unsafe code, can depend on objects not being dropped for correctness. Having a special rule for drop behavior in one very specific case, with no motivation for picking out that case other than an implementation quirk, seems like a really bad idea.

If there's really a desire to optimize closure size at the cost of inconsistent behavior, then early drop should apply to all variables that aren't carried across suspend points, not just unused parameters.

@cramertj
Copy link
Member

@cramertj cramertj commented Apr 18, 2019

@comex there is motivation for async fn behaving differently.

early drop should apply to all variables that aren't carried across suspend points, not just unused parameters.

I don't particularly disagree with this, but it would be an incredibly complex and taxing addition to the language, especially this late in the process.

@cramertj
Copy link
Member

@cramertj cramertj commented Apr 18, 2019

(it would also be much more user-visible, and would likely impact a significant amount of code, unlike the current proposed change)

@CAD97
Copy link
Contributor

@CAD97 CAD97 commented Apr 19, 2019

FWIW, I expect parameters to behave as a "magic" let «pat» = get_fn_param_from_calling_convention!(). This would mean that a parameter that is explicitly ignored (_: «type») would drop "immediately", as happens for let _ = «expr», and a "silenced unused" parameter (_«ident»: «type») would drop "late", as happens for let _«ident» = «expr».

Maybe the difference in behavior here is surprising, but we have it, and authors know to keep this in mind when holding an e.g. scope_guard. We have a warning for unused bindings that don't start with an underscore today; if the type has drop glue we can theoretically suggest using either depending on when the user wants the drop glue to run.

I definitely do not expect the addition of the use of a usable binding (that is, one that isn't _) to change drop order. I would consider it a misfeature if that happened. The _ pattern exists as a "trash can" to drop things you don't want anymore into. If I don't use that pattern, I'm ok with it sticking around (and will have an unused lint if it's not prefixed with an underscore).

@najamelan
Copy link
Contributor

@najamelan najamelan commented Apr 19, 2019

(_: «type») would drop "immediately", as happens for let _ = «expr», and a "silenced unused" parameter (_«ident»: «type») would drop "late", as happens for let _«ident» = «expr».

This seems to solve all the performance questions for the price of users having to know that the two syntaxes don't behave the same. I must say I like it. There won't be any free cake whichever way we take this.

@cramertj
Copy link
Member

@cramertj cramertj commented Apr 19, 2019

We discussed this at the last lang team meeting and decided to change the drop order of async fn to match the existing drop order used by non-async functions. One significant motivation was the converting of things like fn foo(_lock: Mutex<()>) { ... } and other similar "just for the Drop effects" arguments to async fn, and that users might be surprised and confused by the difference in behavior.

However, in the case that we're not able to come up with a satisfactory implementation prior to resolving the other blockers for async/await, stabilization, we will temporarily introduce a feature gate against unused non-Copy arguments to async fn to allow stabilization to move forward.

@najamelan
Copy link
Contributor

@najamelan najamelan commented Apr 19, 2019

@cramertj Sorry if this is a noob question, but is the drop order for non-async functions: _ => drop early, _ident => drop late? And will that be consistent behaviour everywhere, with the user being able to opt into perf gain by using just _?

@cramertj
Copy link
Member

@cramertj cramertj commented Apr 19, 2019

And will that be consistent behaviour everywhere, with the user being able to opt into perf gain by using just _?

No, the performance gain being discussed here is not achievable by using _ in argument position. _ bindings in argument position drop at the end of the function, not at the start (or, in this case, prior to the future even having started).

@CAD97
Copy link
Contributor

@CAD97 CAD97 commented Apr 19, 2019

Would that be a viable future change (for sync and async) to put through Crater? (As iirc parameter drop order isn't actually specified and it would make it more consistent with let.)

It doesn't need to be, but it seems desirable on the surface.

@cramertj
Copy link
Member

@cramertj cramertj commented Apr 19, 2019

@CAD97 That would be a breaking change to a pretty significant amount of existing code, a significant chunk of which is probably unsafe (e.g. functions using _: Mutex<()> as a guard, as I mentioned above).

@comex
Copy link
Contributor

@comex comex commented Apr 20, 2019

OTOH, if the behavior were changed in an edition, it would be a relatively straightforward automatic migration...

@davidtwco davidtwco self-assigned this Apr 21, 2019
@davidtwco
Copy link
Member

@davidtwco davidtwco commented Apr 21, 2019

Assigning myself again since I have an open PR for this.

Centril added a commit to Centril/rust that referenced this issue Apr 23, 2019
[wg-async-await] Drop `async fn` arguments in async block

Fixes rust-lang#54716.

This PR modifies the HIR lowering (and some other places to make this work) so that unused arguments to a async function are always dropped inside the async move block and not at the end of the function body.

```
async fn foo(<pattern>: <type>) {
  async move {
  }
} // <-- dropped as you "exit" the fn

// ...becomes...
fn foo(__arg0: <ty>) {
  async move {
    let <pattern>: <ty> = __arg0;
  } // <-- dropped as you "exit" the async block
}
```

However, the exact ordering of drops is not the same as a regular function, [as visible in this playground example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=be39af1a58e5d430be1eb3c722cb1ec3) - I believe this to be an unrelated issue. There is a [Zulip topic](https://rust-lang.zulipchat.com/#narrow/stream/187312-t-compiler.2Fwg-async-await/topic/.2354716.20drop.20order) for this.

r? @cramertj
cc @nikomatsakis
bors added a commit that referenced this issue Apr 23, 2019
[wg-async-await] Drop `async fn` arguments in async block

Fixes #54716.

This PR modifies the HIR lowering (and some other places to make this work) so that unused arguments to a async function are always dropped inside the async move block and not at the end of the function body.

```
async fn foo(<pattern>: <type>) {
  async move {
  }
} // <-- dropped as you "exit" the fn

// ...becomes...
fn foo(__arg0: <ty>) {
  async move {
    let <pattern>: <ty> = __arg0;
  } // <-- dropped as you "exit" the async block
}
```

However, the exact ordering of drops is not the same as a regular function, [as visible in this playground example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=be39af1a58e5d430be1eb3c722cb1ec3) - I believe this to be an unrelated issue. There is a [Zulip topic](https://rust-lang.zulipchat.com/#narrow/stream/187312-t-compiler.2Fwg-async-await/topic/.2354716.20drop.20order) for this.

r? @cramertj
cc @nikomatsakis
Centril added a commit to Centril/rust that referenced this issue Apr 23, 2019
[wg-async-await] Drop `async fn` arguments in async block

Fixes rust-lang#54716.

This PR modifies the HIR lowering (and some other places to make this work) so that unused arguments to a async function are always dropped inside the async move block and not at the end of the function body.

```
async fn foo(<pattern>: <type>) {
  async move {
  }
} // <-- dropped as you "exit" the fn

// ...becomes...
fn foo(__arg0: <ty>) {
  async move {
    let <pattern>: <ty> = __arg0;
  } // <-- dropped as you "exit" the async block
}
```

However, the exact ordering of drops is not the same as a regular function, [as visible in this playground example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=be39af1a58e5d430be1eb3c722cb1ec3) - I believe this to be an unrelated issue. There is a [Zulip topic](https://rust-lang.zulipchat.com/#narrow/stream/187312-t-compiler.2Fwg-async-await/topic/.2354716.20drop.20order) for this.

r? @cramertj
cc @nikomatsakis
Centril added a commit to Centril/rust that referenced this issue Apr 23, 2019
[wg-async-await] Drop `async fn` arguments in async block

Fixes rust-lang#54716.

This PR modifies the HIR lowering (and some other places to make this work) so that unused arguments to a async function are always dropped inside the async move block and not at the end of the function body.

```
async fn foo(<pattern>: <type>) {
  async move {
  }
} // <-- dropped as you "exit" the fn

// ...becomes...
fn foo(__arg0: <ty>) {
  async move {
    let <pattern>: <ty> = __arg0;
  } // <-- dropped as you "exit" the async block
}
```

However, the exact ordering of drops is not the same as a regular function, [as visible in this playground example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=be39af1a58e5d430be1eb3c722cb1ec3) - I believe this to be an unrelated issue. There is a [Zulip topic](https://rust-lang.zulipchat.com/#narrow/stream/187312-t-compiler.2Fwg-async-await/topic/.2354716.20drop.20order) for this.

r? @cramertj
cc @nikomatsakis
@bors bors closed this in #59823 Apr 23, 2019
@withoutboats withoutboats mentioned this issue Jun 26, 2019
2 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

10 participants