Stream: t-libs/wg-allocators

Topic: To try or not to try?


Tim Diekmann (Dec 19 2019 at 15:03, on Zulip):

@gnzlbg pointed out, that the current Abort trait isn't a good choice. An allocator should not determine, how collection should handle errors. Instead, the collection needs to express this itself.
We came up with a solution of another generic argument Vec<T, A, OomBehavior>. The third parameter will probably be changed to const E: OomBehavior where enum OomBehavior { Error, Abort }, as soon as default arguments are available for const generics.

This way it's even possible to change the behavior of collection, without touching any allocator at all.
I like to try this out in alloc-wg, but there are two possibilites of implementing this:

  1. depending on the generic, the return type of allocator aware function change.
  2. both collection has try_ methods like try_reserve, but only Vec<_, _, OomBehavior::Abort> has methods like reserve (without try_)

Both solutions has advantages and downsides.

  1. Solution

    • advantages:

      • We already express in the Vec type, how OOM should be handled. No need to to type try_ everytime.
        First solution
    • downsides:

      • Vec with different OomBehavior isn't interchangable, as signatures are not identical
      • We will need a way to convert from Vec<_, _, OomBehavior::Abort>to Vec<_, _, OomBehavior::Error> in order to call a method returning a result instead of abort . This could be done via Into, AsRef, and AsMut
  2. Solution: basically the opposite of the first solutions advantages and downsides

What do you think is the better solution? Why?

gnzlbg (Dec 19 2019 at 15:04, on Zulip):

Vec with different OomBehavior isn't interchangable, as signatures are not identical

What do you mean with interchangeable? If I have some code doing infallible allocations, and I change the Vec type to "become" fallible, then that code would still mostly compile on many cases, but warn, due to the #[must_use] on the results.

gnzlbg (Dec 19 2019 at 15:05, on Zulip):

For most methods that allocate (or all?), the return type changes from () to Result<(),...>

gnzlbg (Dec 19 2019 at 15:06, on Zulip):

One cannot replace the vector type in code that is handling those errors, with one that does not produce errors.

gnzlbg (Dec 19 2019 at 15:06, on Zulip):

One can do that with Abort in that direction, but not in the other.

Tim Diekmann (Dec 19 2019 at 15:08, on Zulip):

You forget the methods returning a value or Self like Vec::with_capacity

gnzlbg (Dec 19 2019 at 15:08, on Zulip):

I think it would be worth it to explore to which point interchangeability solves useful problems, and then once we have some concrete examples, maybe evaluate how to improve them.

gnzlbg (Dec 19 2019 at 15:08, on Zulip):

Hm, yes, with_capacity wouldn't work.

Tim Diekmann (Dec 19 2019 at 15:08, on Zulip):

I'd like to go with the first solution, but I like to hear other people's opinion :)

gnzlbg (Dec 19 2019 at 15:09, on Zulip):

Notice that if you have a Vec with an allocator that uses Abort, and call Vec::push, and then try to interchange the vector type with an allocator that does not implement Abort, the code stops compiling as well.

gnzlbg (Dec 19 2019 at 15:09, on Zulip):

Because this new Vec only supports Vec::try_push, and not Vec::push

gnzlbg (Dec 19 2019 at 15:10, on Zulip):

So I don't think there is a proposed solution that's fully interchangeable.

gnzlbg (Dec 19 2019 at 15:10, on Zulip):

However, if you write all your code with Vec::try_, then it works for all allocators.

Tim Diekmann (Dec 19 2019 at 15:11, on Zulip):

When changing from Abort to Error, it should always fail to compile. That's why you want to change it.
I think interchangeability isn't a major point anyway here

gnzlbg (Dec 19 2019 at 15:12, on Zulip):

I think you are onto something with interchangeability.

gnzlbg (Dec 19 2019 at 15:12, on Zulip):

I just wish we had some concrete examples collected somewhere. It might be easier to look at the impact of the different approaches by just trying them out in small snippets.

Tim Diekmann (Dec 19 2019 at 15:13, on Zulip):

Currently, most people don't care about OOM. It occures rarely in the common use case and it's hard to handle in std

Tim Diekmann (Dec 19 2019 at 15:13, on Zulip):

So finding an example project will be hard

Tim Diekmann (Dec 19 2019 at 15:15, on Zulip):

But you can throw the new vector in nearly all crates and watch the compile errors :D

Tim Diekmann (Dec 19 2019 at 21:26, on Zulip):

I found another advantage for the first solution: It's possible to extend this, when adding more behaviors like Panic or Option. We could even go crazy and implement the behavior on the trait itself. This would correspond to something like "policy-based design" described by Andrei Alexandrescu in "Modern C++ Design"

Tim Diekmann (Dec 19 2019 at 21:31, on Zulip):

However, I think implementing the behavior on the trait requires GATs for the return type

John Ericson (Dec 19 2019 at 23:58, on Zulip):

I think 2 is better

John Ericson (Dec 19 2019 at 23:59, on Zulip):

No GATs and better inference

John Ericson (Dec 19 2019 at 23:59, on Zulip):

And easy to write the abort ones in terms of the try_ ones.

Tim Diekmann (Dec 19 2019 at 23:59, on Zulip):

GATs are not required in either solutions

Tim Diekmann (Dec 20 2019 at 00:01, on Zulip):

@John Ericson What do you think about using a generic parameter in general instead of the Alloc trait?

John Ericson (Dec 20 2019 at 00:16, on Zulip):

Yes that is better. It is indeed not a property of the allocator

John Ericson (Dec 20 2019 at 00:17, on Zulip):

But of the collection

John Ericson (Dec 20 2019 at 00:18, on Zulip):

And a collection can pass the parameter to another type it uses, just like it does work the allocator

Lokathor (Dec 20 2019 at 03:37, on Zulip):

I think OOM is hard to handle because we make it hard to handle

Lokathor (Dec 20 2019 at 03:37, on Zulip):

and probably most OOM isn't like actual exhausted memory, but an accidentally crazy amount of memory being requested

Tim Diekmann (Dec 20 2019 at 10:12, on Zulip):

I found another downside for the First solution: the documentation is bloated up, as every allocating method is listed twice. This probably will lead to a lot of confusion, esp. for unexperimented users.

Tim Diekmann (Dec 20 2019 at 10:13, on Zulip):

I guess I will implement both on alloc-wg on different branches

Lokathor (Dec 21 2019 at 03:17, on Zulip):

With my own crates I've found it a useful pattern for the non-try version to just say "As try_foo but it unwraps the failure." or something very curt like that. You could probably do a similar thing here and just say that the OOM handler is triggered on failure.

Tim Diekmann (Dec 21 2019 at 19:14, on Zulip):

That's what I already do, but when two methods are called the same, it's hard to track, which method is implemented on which behavior (although it's intuitive, that the Error-variant will return a Result). The more I play around with it or think about it, the more I like the second solution (try-variants)

Lokathor (Dec 21 2019 at 20:13, on Zulip):

Yeah just become extremely rigid with the naming convention. Every foo op is likely to be just

pub fn foo(args) -> Out {
  try_foo(args)
    .unwrap_or_else(|| handle_alloc_error())
}

And then all the thinking happens in the try variant.

John Ericson (Dec 21 2019 at 20:14, on Zulip):

Hehe well handle_alloc_error needs the layout

John Ericson (Dec 21 2019 at 20:15, on Zulip):

That is why we should probably return Result<T, (Alloc:Error, NonZeroLayout)>

John Ericson (Dec 21 2019 at 20:16, on Zulip):

And then we literally will do the same thing every time

John Ericson (Dec 21 2019 at 20:16, on Zulip):

And can write a macro for it

Tim Diekmann (Dec 21 2019 at 20:36, on Zulip):

That's why we have CollectionAllocError, which stores the layout where necessary

John Ericson (Dec 21 2019 at 20:37, on Zulip):

Right but things like Box don't use it, because no capacity stuff

John Ericson (Dec 21 2019 at 20:37, on Zulip):

Maybe we need to call what we have VecLikeCollectionError

John Ericson (Dec 21 2019 at 20:38, on Zulip):

And make the OOM variant be CollectionAllocError which is basically the tuple

John Ericson (Dec 21 2019 at 20:38, on Zulip):

And add the From instance

John Ericson (Dec 21 2019 at 20:38, on Zulip):

Collections that use Box will especially benefit

John Ericson (Dec 21 2019 at 20:39, on Zulip):

Same for those that use Rc and Arc

Tim Diekmann (Dec 22 2019 at 19:24, on Zulip):

Hmm, OomBehavior? OomPolicy? OomHandling?

Lokathor (Dec 22 2019 at 19:38, on Zulip):

if it's "handle alloc error" and we have an "oom handler", then "oom handling" fits the other names so far.

Tim Diekmann (Dec 22 2019 at 19:39, on Zulip):

is handle_alloc_error stable?

Lokathor (Dec 22 2019 at 19:39, on Zulip):

yeah https://doc.rust-lang.org/std/alloc/fn.handle_alloc_error.html ?

Tim Diekmann (Dec 22 2019 at 19:40, on Zulip):

Okay, then OomHandling sounds right

gnzlbg (Jan 07 2020 at 18:24, on Zulip):

@Tim Diekmann

This would correspond to something like "policy-based design" described by Andrei Alexandrescu in "Modern C++ Design"

The line between policy-based design being a good or bad idea in practice is thin. The whole point of the policy is to change some behavior, but if the "semantics" of most type methods depend on the actual policy, then it gets hard to learn what a method actually does.

gnzlbg (Jan 07 2020 at 18:25, on Zulip):

I think for this particular use case its fine, since the policy controls whether some methods exist or not.

gnzlbg (Jan 07 2020 at 18:25, on Zulip):

And not the "behavior" of these methods, so that the docs of the methods does not depend on the policy.

gnzlbg (Jan 07 2020 at 18:26, on Zulip):

In the Rust API docs, depending whether we follow the try_X vs two different X approaches, things might be easier or harder to document.

gnzlbg (Jan 07 2020 at 18:28, on Zulip):

E.g. if we go with the

impl<T, A> Vec<T, A, OomAbort> {
    // Doc for the abort on OOM version
    pub fn push(&mut self, T) { ... }
}
impl<T, A> Vec<T, A, OomHandling> {
    // Doc for the "result-returning" version
    pub fn push(&mut self, T) -> Result<()> { ... }
}

approach, we end up with two push functions in the API docs, with two different doc strings, etc.

gnzlbg (Jan 07 2020 at 18:32, on Zulip):

But you can throw the new vector in nearly all crates and watch the compile errors :D

Notice that one can write code that is polymorphic over these, one sometimes doesn't even need traits, eg., let _ = vec.push(); will work in both cases.

gnzlbg (Jan 07 2020 at 18:33, on Zulip):

For the more general case of taking a collection polymorphically, and propagating errors in the "collection-preferred" way (using panics, or using results), then that's going to need some generics plumbing.

gnzlbg (Jan 07 2020 at 18:34, on Zulip):

But that's the case for both strategies (e.g. try_ has the same problem).

Lokathor (Jan 07 2020 at 18:58, on Zulip):

If ? worked on () type values and simply always became () without the possibility of early return, then it would also let people write v.push()?; for both handling and panicing vecs

gnzlbg (Jan 07 2020 at 19:24, on Zulip):

That would be a cool extension

gnzlbg (Jan 07 2020 at 19:26, on Zulip):

but I don't think this extends to the rest of APIs, e.g., Vec::pop() -> T, where you wouldn't be able to write v.pop()? in both cases.

Lokathor (Jan 07 2020 at 20:06, on Zulip):

but isn't pop always an Option<T>? Any vec can be empty

gnzlbg (Jan 07 2020 at 20:22, on Zulip):

hmm, duh, bad example

Last update: Jan 28 2020 at 01:35UTC