Stream: t-libs/wg-allocators

Topic: Design of `AbortAlloc` not compatible with `try_reserve`


Tim Diekmann (Nov 21 2019 at 18:54, on Zulip):

AbortAlloc is implemented as a wrapper around an arbitrary allocator A. AbortAlloc maps the returend error to handle_alloc_error. So if an OOM occures, there is no way to recover. Thus, AbortAlloc is incompatible with try_reserve. Any thoughts/idea on this?

Erich Gubler (Nov 22 2019 at 00:28, on Zulip):

What do you mean that it's incompatible?

Tim Diekmann (Nov 22 2019 at 06:28, on Zulip):

When calling try_reserve (or any other try_method), you expect to get an allocation error on OOM. AbortAlloc aborts before the function returns.

Tim Diekmann (Nov 22 2019 at 06:28, on Zulip):

This is the desired effect on methods like reserve, but not on try_resersve.

gnzlbg (Nov 28 2019 at 17:01, on Zulip):

Is try_reserve stable ?

gnzlbg (Nov 28 2019 at 17:01, on Zulip):

I think I would prefer to, e.g., get try_ versions of all allocating methods, not only reserve, but all of them, by just using a PanicAlloc that panics, such that the panic can be caught

John Ericson (Nov 28 2019 at 17:01, on Zulip):

I think it is stable

gnzlbg (Nov 28 2019 at 17:02, on Zulip):

The default allocator can abort on OOM

John Ericson (Nov 28 2019 at 17:02, on Zulip):

*unstable

John Ericson (Nov 28 2019 at 17:02, on Zulip):

I tried to prevent it from being stabilized for exactly this reason

gnzlbg (Nov 28 2019 at 17:02, on Zulip):

We can always deprecate it afterwards

gnzlbg (Nov 28 2019 at 17:12, on Zulip):

One situation were this approach wouldn't work is if -C panic=abort.

gnzlbg (Nov 28 2019 at 17:13, on Zulip):

In that case, Vec::try_reserve would still work properly, since no panics occur, but a PanicOnOomAlloc<MyAlloc> adapter would terminate the process.

John Ericson (Nov 28 2019 at 17:14, on Zulip):

@gnzlbg in my PR, I used repr(transparent) so I could cast on and off the wrapper

John Ericson (Nov 28 2019 at 17:15, on Zulip):

So if you want today's try_reserve, you just cast away the PanicOnOomAlloc<_>, and then call try_reserve.

John Ericson (Nov 28 2019 at 17:16, on Zulip):

It's slightly more verbose but I don't really care. I think anyone that's using try_reserve today is much better off without PanicOnOomAlloc anyways.

John Ericson (Nov 28 2019 at 17:17, on Zulip):

try_reserve is a hacky way to make the thing you actually want to do hopefully succeed. But that's racy; you still want to do not panic on the thing itself. And in that case unless you are reserving way before, you probably don't need the try_reserve at all.

gnzlbg (Nov 28 2019 at 17:20, on Zulip):

It's slightly more verbose but I don't really care. I think anyone that's using try_reserve today is much better off without PanicOnOomAlloc anyways.

@John Ericson without or with ?

John Ericson (Nov 28 2019 at 17:20, on Zulip):

without

gnzlbg (Nov 28 2019 at 17:20, on Zulip):

So you are in favour of only doing try_reserve ?

John Ericson (Nov 28 2019 at 17:20, on Zulip):

@gnzlbg if you scroll up in that issue you posted in, you can read me describe the stuff I am saying here with more words

gnzlbg (Nov 28 2019 at 17:20, on Zulip):

ok i will

John Ericson (Nov 28 2019 at 17:21, on Zulip):

@gnzlbg I mean that PanicOnOomAlloc definitely should exist, but you shouldn't be mixing sometimes panicking and somtimes not panicking

Simon Sapin (Nov 28 2019 at 17:21, on Zulip):

The default allocator can abort on OOM

No, in today’s standard library it’s Vec and other callers that check for errors and call handle_alloc_error which in turn defaults to abort.

gnzlbg (Nov 28 2019 at 17:23, on Zulip):

@John Ericson this one: https://github.com/rust-lang/rust/issues/48043#issuecomment-501416660 ?

gnzlbg (Nov 28 2019 at 17:23, on Zulip):

Im not sure I fully understand what you are propossing, it feels as if you want all Vec methods to return a Result

Simon Sapin (Nov 28 2019 at 17:24, on Zulip):

AbortAlloc is implemented as a wrapper around an arbitrary allocator A. AbortAlloc maps the returend error to handle_alloc_error. So if an OOM occures, there is no way to recover. Thus, AbortAlloc is incompatible with try_reserve. Any thoughts/idea on this?

Yes, making the choice to abort or not be part of the type signature is incompatible with having it only as part of the choice of method being called on a given type. I personally prefer the latter.

gnzlbg (Nov 28 2019 at 17:24, on Zulip):

No, in today’s standard library it’s Vec and other callers that check for errors and call handle_alloc_error which in turn defaults to abort.

@Simon Sapin yes, but I can provide a #[global_allocator] that aborts on OOM or panics instead by using AbortOnOOM<std::alloc::Heap>; right ?

John Ericson (Nov 28 2019 at 17:26, on Zulip):

I think https://github.com/rust-lang/rust/issues/48043#issuecomment-409967652 is the first in my thread. It's somewhat hard to follow the convo rereading it again I'll admit. These sorts of discussions tended to be strewn around a bunch of issues, sadly.

John Ericson (Nov 28 2019 at 17:26, on Zulip):

@gnzlbg yes there should be a try_ version of every method that returns a result.

gnzlbg (Nov 28 2019 at 17:27, on Zulip):

Ah gotcha

Simon Sapin (Nov 28 2019 at 17:27, on Zulip):

There’s nothing technically stopping you from aborting inside your #[global_allocator], but https://doc.rust-lang.org/std/alloc/trait.GlobalAlloc.html#errors “encourages” not to

gnzlbg (Nov 28 2019 at 17:27, on Zulip):

So do you want try_clone as well in the Clone trait ?

John Ericson (Nov 28 2019 at 17:27, on Zulip):

Probably yes

gnzlbg (Nov 28 2019 at 17:28, on Zulip):

Like if I extend a Vec<T>, and try_reserve succeeded, but extending requires calls to T::clone, and those T::clone allocate, then, you are screwed

John Ericson (Nov 28 2019 at 17:28, on Zulip):

exactly!

Simon Sapin (Nov 28 2019 at 17:28, on Zulip):

I think the idea of an AbortOnOOM wrapper type is flawed

gnzlbg (Nov 28 2019 at 17:28, on Zulip):

What's the goal of AbortOnOOM ?

John Ericson (Nov 28 2019 at 17:28, on Zulip):

the only way to systematically ensure your program will not crash is to never panic and ? the Result --- ideomatic error handling.

Simon Sapin (Nov 28 2019 at 17:29, on Zulip):

I don’t know, you mentioned it

gnzlbg (Nov 28 2019 at 17:29, on Zulip):

I mentioned PanicOnOOM

gnzlbg (Nov 28 2019 at 17:29, on Zulip):

which is quite differently

Simon Sapin (Nov 28 2019 at 17:29, on Zulip):

https://rust-lang.zulipchat.com/#narrow/stream/197181-t-libs.2Fwg-allocators/topic/Design.20of.20.60AbortAlloc.60.20not.20compatible.20with.20.60try_reserve.60/near/182124881

gnzlbg (Nov 28 2019 at 17:29, on Zulip):

I was asking if that is a possibility

John Ericson (Nov 28 2019 at 17:30, on Zulip):

the non-try methods would require the allocator parameter to have ! as the error type, which means that alllocation failure is "impossible", which means something panics. PanicOnOOM would have the ! error type.

gnzlbg (Nov 28 2019 at 17:30, on Zulip):

and if your goal is to abort on any OOM, it is quite simple

gnzlbg (Nov 28 2019 at 17:30, on Zulip):

and doesn't rely on anybody calling the handle_alloc_error

Simon Sapin (Nov 28 2019 at 17:30, on Zulip):

It’s technically possible. IMO it’s not a good idea.

gnzlbg (Nov 28 2019 at 17:30, on Zulip):

the same applies if you want to panic on any OOM

gnzlbg (Nov 28 2019 at 17:30, on Zulip):

Why?

Simon Sapin (Nov 28 2019 at 17:31, on Zulip):

Why would you panic or abort if the caller has specifically written code to handle both outcomes of try_reserve?

Simon Sapin (Nov 28 2019 at 17:31, on Zulip):

That just seems hostile to me

gnzlbg (Nov 28 2019 at 17:32, on Zulip):

Because if I'm writing #[global_allocator], i'm in charge of building the final binary, and I can do whatever I want?

John Ericson (Nov 28 2019 at 17:32, on Zulip):

@gnzlbg or you can read https://github.com/rust-lang/rust/pull/60703 where I implemented everything I've talked about

gnzlbg (Nov 28 2019 at 17:32, on Zulip):

Like, maybe I just want my binary to fail as fast as possible on OOM without any kind of cleanup - its kind of up to me

John Ericson (Nov 28 2019 at 17:32, on Zulip):

@Simon Sapin with the associated error type try_* would return Result<_, !>, with PanicOnOOM so the user gets a heads up that their code isn't doing what they through via dead code warnings.

gnzlbg (Nov 28 2019 at 17:33, on Zulip):

One way to implement all standard collections more "concisely" would be to wrap the user allocator in a HandleAllocError<Alloc> wrapper internally, that always calls handle_alloc_error on OOM.

Simon Sapin (Nov 28 2019 at 17:33, on Zulip):

@John Ericson I’m sorry, you and I already had this discussion a couple times and I don’t have anything new to add

John Ericson (Nov 28 2019 at 17:34, on Zulip):

The things you are telling @gnzlbg seem like the same things you have told me?

gnzlbg (Nov 28 2019 at 17:34, on Zulip):

@John Ericson I think the difference is that I'm not suggesting for everything to return Result

John Ericson (Nov 28 2019 at 17:34, on Zulip):

I'll not @ you then and at @ @gnzlbg instead, I guess

Simon Sapin (Nov 28 2019 at 17:34, on Zulip):

I meant about Result<_, !> everywhere

John Ericson (Nov 28 2019 at 17:35, on Zulip):

@gnzlbg so without a bunch of new try_ methods, what are you proposing for writing code that never blows up on allocation failure?

gnzlbg (Nov 28 2019 at 17:35, on Zulip):

Vec<T, PanicOnOOM<Alloc>>

John Ericson (Nov 28 2019 at 17:35, on Zulip):

that still panics?

gnzlbg (Nov 28 2019 at 17:35, on Zulip):

it immediately panics, always

John Ericson (Nov 28 2019 at 17:35, on Zulip):

is the application supposed to catch the panic?

gnzlbg (Nov 28 2019 at 17:36, on Zulip):

Yes, it could, if it wanted

John Ericson (Nov 28 2019 at 17:36, on Zulip):

Say you don't want to panic or abort

gnzlbg (Nov 28 2019 at 17:36, on Zulip):

Only works with unwinding

gnzlbg (Nov 28 2019 at 17:36, on Zulip):

If you don't have unwinding, or don't want to unwind, this won't work for you

John Ericson (Nov 28 2019 at 17:36, on Zulip):

that's why I want try_*

John Ericson (Nov 28 2019 at 17:37, on Zulip):

I want my code to not fail, and so I don't write any panics, and I cannot unwind so I do panic == abort

gnzlbg (Nov 28 2019 at 17:37, on Zulip):

But if you want to support Vec<T> failing to resize, T::clone failing, etc. , a global allocator that panics on OOM, and an allocator wrapper that panics on OOM, give you that if you have unwinding

gnzlbg (Nov 28 2019 at 17:37, on Zulip):

And it gives you that without having Result everywhere

John Ericson (Nov 28 2019 at 17:37, on Zulip):

Isn't Result everywhere the ideomatic way of doing error handling in rust?

John Ericson (Nov 28 2019 at 17:37, on Zulip):

and catching panics not?

gnzlbg (Nov 28 2019 at 17:37, on Zulip):

Not for allocation errors

John Ericson (Nov 28 2019 at 17:38, on Zulip):

why is allocation different?

gnzlbg (Nov 28 2019 at 17:38, on Zulip):

Because somebody decided so

gnzlbg (Nov 28 2019 at 17:38, on Zulip):

That decision happened long time ago, and we can't change it

John Ericson (Nov 28 2019 at 17:38, on Zulip):

Right we have some legacy cruft, but I rather things be different for principled reasons.

gnzlbg (Nov 28 2019 at 17:38, on Zulip):

You can write RFCs for adding a lot of try_methods, and maybe they'll go through

gnzlbg (Nov 28 2019 at 17:39, on Zulip):

But for me, unwinding is ok, and a small Alloc<A> wrapper makes allocation fallible, without having to change anything or add any APIs

gnzlbg (Nov 28 2019 at 17:39, on Zulip):

Or rely on people actually using those APIs

John Ericson (Nov 28 2019 at 17:39, on Zulip):

@gnzlbg so what makes this difficult is to me PanicOnOOM, Result, and try_* all go together.

John Ericson (Nov 28 2019 at 17:39, on Zulip):

I need all 3 of them to make the coherent argument

gnzlbg (Nov 28 2019 at 17:40, on Zulip):

I don't need Result and try_

John Ericson (Nov 28 2019 at 17:40, on Zulip):

What does PanicOnOOM do without try_*?

gnzlbg (Nov 28 2019 at 17:40, on Zulip):

because I have unwinding, and can use it to handle OOM

John Ericson (Nov 28 2019 at 17:40, on Zulip):

Say you diddn't use PanicOnOOM. What would happen?

gnzlbg (Nov 28 2019 at 17:40, on Zulip):

PanicOom<A: Alloc>: Alloc is an allocator wrapper, that forwards arguments and returns from / to A, but if a return is a null pointer due to an allocation error, it immediately panics.

John Ericson (Nov 28 2019 at 17:41, on Zulip):

So what happens if I try to do Vec<A> without Vec<PanicOnOOM<A>

John Ericson (Nov 28 2019 at 17:41, on Zulip):

and A returns a null pointer

gnzlbg (Nov 28 2019 at 17:41, on Zulip):

If A forwards to the #[global_allocator] then that would panic, if you make it a PanicOnOOM

gnzlbg (Nov 28 2019 at 17:42, on Zulip):

If A is its own thing, it would call handle_alloc_err at some point, which you can configure to also panic

John Ericson (Nov 28 2019 at 17:42, on Zulip):

panic or abort, right?

gnzlbg (Nov 28 2019 at 17:43, on Zulip):

but then you are relying on Vec always calling handle_alloc_err when it should

gnzlbg (Nov 28 2019 at 17:43, on Zulip):

You can provide your own handler

gnzlbg (Nov 28 2019 at 17:43, on Zulip):

So in my case, I would provide a PanicOnOom global allocator, a handler that panics, etc.

John Ericson (Nov 28 2019 at 17:43, on Zulip):

@gnzlbg I don't feel very motivated by your PanicOnOOM

John Ericson (Nov 28 2019 at 17:43, on Zulip):

So it always panics

John Ericson (Nov 28 2019 at 17:43, on Zulip):

and we just muddle who has the oblilgation

gnzlbg (Nov 28 2019 at 17:43, on Zulip):

yes, and I can catch those panics, if I care about that, and where I care about that

John Ericson (Nov 28 2019 at 17:43, on Zulip):

it would be nice to say "Vec, you don't need to worry about null pointers because that is already taken care of"

John Ericson (Nov 28 2019 at 17:44, on Zulip):

but you cannot rely on the allocator using PanicOnOOM so Vec has to be defensive anyways

John Ericson (Nov 28 2019 at 17:44, on Zulip):

so you get both sides panicking

John Ericson (Nov 28 2019 at 17:44, on Zulip):

what's the point?

gnzlbg (Nov 28 2019 at 17:44, on Zulip):

That there are collections that are not Vec that I also use

gnzlbg (Nov 28 2019 at 17:44, on Zulip):

inside the 100s of my crates I depend on

gnzlbg (Nov 28 2019 at 17:44, on Zulip):

and I don't trustthem

John Ericson (Nov 28 2019 at 17:44, on Zulip):

But they either need to return Result, or panic themselves

John Ericson (Nov 28 2019 at 17:45, on Zulip):

the type system forces this

John Ericson (Nov 28 2019 at 17:45, on Zulip):

Are you worried about those other collections just not handling null pointers at all?

John Ericson (Nov 28 2019 at 17:46, on Zulip):

Then the solution is to make sure that no collection needs to worry about nullness

John Ericson (Nov 28 2019 at 17:46, on Zulip):

@gnzlbg here's the thing, even if every collection is to be used with OOM panics and unwinding, when building collections it's much easier to use Result

John Ericson (Nov 28 2019 at 17:47, on Zulip):

You don't have to be a careful C programmer and remember all your null pointers

John Ericson (Nov 28 2019 at 17:47, on Zulip):

and collections can be made out of other collections, etc etc

John Ericson (Nov 28 2019 at 17:47, on Zulip):

therefore I don't recognize there being a clear division of labor between "collections code" and "applications code"

John Ericson (Nov 28 2019 at 17:48, on Zulip):

It's best if we just add all the try_ methods and associated error type so we can safely create all these other collections, and the responsibility for error handling is clearly laid out and no one needs to be defense without help from the compiler. Nobody is being forced to use the try_* methods, remember.

Tim Diekmann (Nov 28 2019 at 19:01, on Zulip):

I think the idea of an AbortOnOOM wrapper type is flawed

Yes it is, that's the reason why I removed it from alloc-wg. The thing is, that it will always aborts (or panics in the case of PanicOnOom) as soon as an allocation fails, even before try_reserve can return a result.

Personally I'm not fine with relying on unwinding on OOM. That defeats the purpose on returning a Result. Personally I also think, that the current design of the collections isn't that great, but we can't change this anyway, so we shouldn't bother.

John Ericson (Nov 28 2019 at 19:06, on Zulip):

@Tim Diekmann I would be happy to contribute my PR to that library

John Ericson (Nov 28 2019 at 19:07, on Zulip):

I don't get all this "ship has sailed" talk when there is no problem with adding a bunch of new methods

John Ericson (Nov 28 2019 at 19:08, on Zulip):

And there could be some lang thing down the road for Result<T,!> to T

John Ericson (Nov 28 2019 at 19:08, on Zulip):

So it's not like 2x methods is necessary a permanent state of affairs

Tim Diekmann (Nov 28 2019 at 19:09, on Zulip):

Sure, go ahead and let's see, how it turns out. The crate tries to collect proposals of the WG to try them, not to be merged as the new liballoc.

John Ericson (Nov 28 2019 at 19:09, on Zulip):

Ok thanks

Tim Diekmann (Nov 28 2019 at 19:09, on Zulip):

I don't get all this "ship has sailed" talk

Me neither

John Ericson (Nov 28 2019 at 19:13, on Zulip):

@Tim Diekmann now I'm confused :) Didn't you just say "but we can't change this anyway"? Or were you referring to something more specific?

Tim Diekmann (Nov 28 2019 at 19:13, on Zulip):

A few commits back you can see the design with AbortAlloc and the bound for AllocRed<Error = !> for infallible methods .

Tim Diekmann (Nov 28 2019 at 19:14, on Zulip):

Wait a second searching for a cite

John Ericson (Nov 28 2019 at 19:14, on Zulip):

OK

Tim Diekmann (Nov 28 2019 at 19:20, on Zulip):

Can't find it right now. However I don't see why the collections defaults to abort instead of panicking. Or even don't return Result

John Ericson (Nov 28 2019 at 19:20, on Zulip):

OK, sweet!

John Ericson (Nov 28 2019 at 19:23, on Zulip):

@Tim Diekmann oh, looks like alloc-wg does a lot of this stuff already?! Sorry, I am comming back to these things after quite a while

Tim Diekmann (Nov 28 2019 at 19:51, on Zulip):

No problem :D

Tim Diekmann (Nov 28 2019 at 19:52, on Zulip):

As mentioned before: I tried to collect many proposals so it turns out how much sense everything makes.

gnzlbg (Nov 29 2019 at 11:55, on Zulip):

Yes it is, that's the reason why I removed it from alloc-wg. The thing is, that it will always aborts (or panics in the case of PanicOnOom) as soon as an allocation fails, even before try_reserve can return a result.

That is kind of the point right? What can you do to prevent this from happening? e.g. I can write an AbortOnOom wrapper that sets the #[global_allocator] to abort on OOM, and there is nothing the standard library can do against that.

gnzlbg (Nov 29 2019 at 11:58, on Zulip):

For something like PanicOnOom, we can protect against this by saying that a GlobalAlloc that unwinds is UB, and e.g. making the GlobalAlloc hooks nounwind, to be able to optimize under this assumption.

gnzlbg (Nov 29 2019 at 11:59, on Zulip):

But if you write a library that uses Vec::try_reserve, you are not guaranteed a Result on allocation error, because your allocator might abort the process if that happens, and there is nothing you can do about that.

gnzlbg (Nov 29 2019 at 12:00, on Zulip):

On Linux, you might get even an Ok, and only when you touch the memory, the OOM-killer kills your process

gnzlbg (Nov 29 2019 at 12:01, on Zulip):

So the best that Vec::try_reserve can do is guarantee that handle_alloc_error won't be called

gnzlbg (Nov 29 2019 at 12:02, on Zulip):

which is very different from the guarantee that allocation errors will be reported as Err, there is no way to guarantee that in Rust

gnzlbg (Nov 29 2019 at 12:02, on Zulip):

you need the allocator and the OS to conspire together with Vec::try_reserve to support those semantics

gnzlbg (Nov 29 2019 at 12:04, on Zulip):

(the goal of Vec::try_reserve is to support supporting those semantics when all those components align)

Tim Diekmann (Nov 29 2019 at 12:10, on Zulip):

That is kind of the point right?

I tried to modify the collection API, that allocating methods like Vec::reserve can only be used for infallible allocators (AllocRef<Error = !>). To maintain backwards combatibility, I introduced the wrapping allocator AbortAlloc.

Tim Diekmann (Nov 29 2019 at 12:11, on Zulip):

To achieve this, the design of AbortAlloc simply didn't work out

gnzlbg (Nov 29 2019 at 12:11, on Zulip):

Ah yes, that won't work

gnzlbg (Nov 29 2019 at 12:12, on Zulip):

What I think could be done is provide a different FallibleVec in libstd, where all allocating methods return Result<(), AllocRef::Error> or some other error type.

Tim Diekmann (Nov 29 2019 at 12:12, on Zulip):

This brings me to two points:
- Does an infallible generic allocator even makes sense?
- I think this could only be solved with an additional trait: InfallibleAlloc (and InfallibleRealloc?)

gnzlbg (Nov 29 2019 at 12:12, on Zulip):

Then, Vec can be implemented on top of such a FallibleVec type by just .unwrap()ing everything

Tim Diekmann (Nov 29 2019 at 12:13, on Zulip):

What I think could be done is provide a different FallibleVec in libstd

Right, this would be another possibility

gnzlbg (Nov 29 2019 at 12:13, on Zulip):

If we ever get a prelude::v2 in a future edition, we could add a Vec type alias for FallibleVec

gnzlbg (Nov 29 2019 at 12:13, on Zulip):

and offer the old Vec through a std::v1::collections::Vec.

gnzlbg (Nov 29 2019 at 12:13, on Zulip):

Only for crates on the new edition, on the old edition, the prelude would just point to v1, so that would be a backward compatible change.

gnzlbg (Nov 29 2019 at 12:14, on Zulip):

Code in the new edition interfacing with APIs from crates using the old edition using Vec, would need to use std::v1::collection::Vec instead, so this might end up being quite a pain, but... is doable.

Tim Diekmann (Nov 29 2019 at 12:14, on Zulip):

I think introducing FallibleVec wouldn't be the optimal solution, as Vec isn't the only collection which allocates. We would need a fallible variant for all collections

gnzlbg (Nov 29 2019 at 12:14, on Zulip):

Yes

gnzlbg (Nov 29 2019 at 12:14, on Zulip):

but we can do that one collection at a time

gnzlbg (Nov 29 2019 at 12:15, on Zulip):

without breaking backward compatibility, and just changing what the defaults are between editions, while providing both version on all editions

Tim Diekmann (Nov 29 2019 at 12:15, on Zulip):

What do you think on introducing liballoc::alloc::Infallible?

Tim Diekmann (Nov 29 2019 at 12:15, on Zulip):

as trait

gnzlbg (Nov 29 2019 at 12:17, on Zulip):

What does that mean ?

gnzlbg (Nov 29 2019 at 12:17, on Zulip):

An alloc that impls Infallible never fails ?

Tim Diekmann (Nov 29 2019 at 12:17, on Zulip):

Kind of, an alloc, which will never return on OOM

Tim Diekmann (Nov 29 2019 at 12:19, on Zulip):

But I don't know if we end up with the same problem...

Tim Diekmann (Nov 29 2019 at 12:21, on Zulip):

We shouldn't forget that most of the users will never use or write a custom allocator or even use try_reserve.

Tim Diekmann (Nov 29 2019 at 12:22, on Zulip):

So we shouldn't clutter the namespace with too much types.

Tim Diekmann (Nov 29 2019 at 12:25, on Zulip):

But the question remains: How much sense does a generic infallible allocator make? Won't all allocator OOM at a time without introducing additional constraints?

gnzlbg (Nov 29 2019 at 15:38, on Zulip):

I think that until somebody actually tries to implement this, and shows how it is used to implement Vec, and then how Vec is end up being used, it will be very hard to tell.

gnzlbg (Nov 29 2019 at 15:40, on Zulip):

I'm skeptic that it will be as useful as @John Ericson says, but I'm also skeptic about this being completely useless, as @Simon Sapin seems to believe. I'll probably only be able to tell when I see both options next to each other.

gnzlbg (Nov 29 2019 at 15:40, on Zulip):

Vec::try_reserve is a solution that's a bit "in-between", and maybe it is the best solution there is. No idea yet.

John Ericson (Nov 30 2019 at 04:02, on Zulip):

@Tim Diekmann I am working on my allog-wg, why does Box use unchecked stuff?

Tim Diekmann (Nov 30 2019 at 10:37, on Zulip):

That's a good catch. I forgot to remove it when I removed AbortAlloc, thanks

Tim Diekmann (Nov 30 2019 at 10:56, on Zulip):

You basically reverted my AbortAlloc :D

John Ericson (Nov 30 2019 at 14:24, on Zulip):

@Tim Diekmann I tried to find AbortAlloc but didn't, thanks

Tim Diekmann (Nov 30 2019 at 14:25, on Zulip):

I removed it at 0.7.0. You can find it in the docs for 0.6.0

John Ericson (Nov 30 2019 at 14:25, on Zulip):

@Tim Diekmann OK I read your explanation, thanks, and I think I get it now

John Ericson (Nov 30 2019 at 14:26, on Zulip):

So this goes back to what @gnzlbg said about it might be best to change the meaning of try_reserve

Tim Diekmann (Nov 30 2019 at 14:26, on Zulip):

@gnzlbg and I thought about another solution yesterday, you can see the discussion above :)

John Ericson (Nov 30 2019 at 14:27, on Zulip):

FallibleVec?

Tim Diekmann (Nov 30 2019 at 14:27, on Zulip):

I think the current meaning of try_reserve is intuitive, don't you?

John Ericson (Nov 30 2019 at 14:27, on Zulip):

Well let's just talk about FallibleVec for a second

Tim Diekmann (Nov 30 2019 at 14:28, on Zulip):

- FallibleVec, but this introduces a lot more structures
- Infallible as another alloc trait is another option

Tim Diekmann (Nov 30 2019 at 14:28, on Zulip):

sure!

John Ericson (Nov 30 2019 at 14:28, on Zulip):

So we have this split ecosytem that's part "I don't care about allocation failure" and "I really care about allocation failure"

Tim Diekmann (Nov 30 2019 at 14:28, on Zulip):

Yeah, true

John Ericson (Nov 30 2019 at 14:28, on Zulip):

I want there to be libraries that support both

Tim Diekmann (Nov 30 2019 at 14:29, on Zulip):

Me too. The best case would be an API which supports both

John Ericson (Nov 30 2019 at 14:29, on Zulip):

My best idea for that is the with the foo = try_foo.infallible_unwrap() pattern

John Ericson (Nov 30 2019 at 14:29, on Zulip):

Is FallibleVec supposed to be a whole different struct?

John Ericson (Nov 30 2019 at 14:30, on Zulip):

Maybe a more compatible with the above alternative would be an extension trait that you don't get in the prelude, so regular users aren't confused?

John Ericson (Nov 30 2019 at 14:30, on Zulip):

Also I think a type Panic = ! would make things a lot more intuitive?

Tim Diekmann (Nov 30 2019 at 14:30, on Zulip):

Maybe a more compatible with the above alternative would be an extension trait that you don't get in the prelude, so regular users aren't confused?

I thought about that, too

Tim Diekmann (Nov 30 2019 at 14:31, on Zulip):

Also I think a type Panic = ! would make things a lot more intuitive?

Yes, indeed. That's why I really liked the AllocRef<Error = !> approach

John Ericson (Nov 30 2019 at 14:31, on Zulip):

If you see fn try_reserve(&mut self) -> Result<(), Panic>, <- after subsitutition, I think it makes a bit more sense what is going on.

John Ericson (Nov 30 2019 at 14:35, on Zulip):

@Tim Diekmann so back to the test failures, is this just a matter of changing the tests?

Tim Diekmann (Nov 30 2019 at 14:35, on Zulip):

In the current state (0.7.0), you can just call reserve and it will abort on OOM. However, when calling try_reserve it will return Result<(), Err>. If your allocator never fails, it still returns Return<(), !>, which is pretty intuitive. The user, who don't care about fallible collections can still use reserve. Do I get you right, that you want to introduce all the try_ methods in an extension traits excluded from the prelude?

Tim Diekmann (Nov 30 2019 at 14:36, on Zulip):

You can fix this when exchanging AbortAlloc<Global> (forgot your name :) ) with Global. This requires a bit of dancing for the String tests, but it's possible. Let me search the commit, where I fixed this test - I also had those failures before.

John Ericson (Nov 30 2019 at 14:38, on Zulip):

@Tim Diekmann also in general, sorry for making a bug fuss and not finding the commits that showed you had tried all of the stuff in that PR.

John Ericson (Nov 30 2019 at 14:39, on Zulip):

I've wasted both our time :grimacing:

John Ericson (Nov 30 2019 at 14:39, on Zulip):

I found the tests commit now

Tim Diekmann (Nov 30 2019 at 14:39, on Zulip):

for Vec: https://github.com/TimDiekmann/alloc-wg/pull/10/commits/dfa9939e4bd6007fcd9d5a89537d3081ef1634a7
for String: https://github.com/TimDiekmann/alloc-wg/pull/10/commits/06fff0335cc00abf195659056cb52286b43b43d5

Tim Diekmann (Nov 30 2019 at 14:39, on Zulip):

Me too :yum:

Tim Diekmann (Nov 30 2019 at 14:40, on Zulip):

I think you better "waste" my time for one hour, than don't participate at all :)
Now you know about my design decisions, so don't worry :+1:

Tim Diekmann (Nov 30 2019 at 14:41, on Zulip):

Don't know if the second commit is required now, but the first definitely fixed your test case

Tim Diekmann (Nov 30 2019 at 14:43, on Zulip):

But back to the design: I don't think we should stick with AbortAlloc/PanicAdapter at all. Do you want to try out, how the extension trait works out?
Personally, I'd love to have more traits on collections anyway, but this is another story.

Tim Diekmann (Nov 30 2019 at 14:44, on Zulip):

Like generic traits for slicing, random access, len etc.

Tim Diekmann (Nov 30 2019 at 14:44, on Zulip):

But this wasn't done yet due to the lack of GAT

John Ericson (Nov 30 2019 at 15:06, on Zulip):

GAT?

Tim Diekmann (Nov 30 2019 at 15:07, on Zulip):

Generic associated type

John Ericson (Nov 30 2019 at 15:07, on Zulip):

ah ok

John Ericson (Nov 30 2019 at 15:07, on Zulip):

yeah it is ugly without it

John Ericson (Nov 30 2019 at 15:07, on Zulip):

So if you like type Panic = !, what is the remaing issue with the adapter?

John Ericson (Nov 30 2019 at 15:08, on Zulip):

Or really, I am much more fond of the Error = ! bounds than the adapter itself

John Ericson (Nov 30 2019 at 15:08, on Zulip):

I combined my work with the reverts or reverts, and the tests pass now

John Ericson (Nov 30 2019 at 15:08, on Zulip):

fwiw

Tim Diekmann (Nov 30 2019 at 15:09, on Zulip):

Calling try_reserve with AbortAlloc has an unintuitive behavior: It aborts on OOM. When AbortAlloc is the default for Vec, try_reserve is pretty useless for non-custom allocators

Tim Diekmann (Nov 30 2019 at 15:11, on Zulip):

The expected behavior would be, that it aborts on reserve and returns a result on try_reserve

Tim Diekmann (Nov 30 2019 at 15:11, on Zulip):

But without an additional trait or more methods in ReallocRef this isn't possible

John Ericson (Nov 30 2019 at 15:12, on Zulip):

@Tim Diekmann Are you just worried about try_reserve, or all such try_* methods?

Tim Diekmann (Nov 30 2019 at 15:12, on Zulip):

All try_* methods

John Ericson (Nov 30 2019 at 15:13, on Zulip):

So we could do some associated type monstrosity to fix this

Tim Diekmann (Nov 30 2019 at 15:13, on Zulip):

But try_reserve is the most intuitive example. What's the point on having Vec::<T, AbortAlloc<Global>>::try_reserve if it only checks for capacity overflow?

Tim Diekmann (Nov 30 2019 at 15:14, on Zulip):

so, this would be

let mut v = vec![];
v.try_reserve(10)?; // may abort
John Ericson (Nov 30 2019 at 15:15, on Zulip):

Yes, the point is just so reserve can be written in terms of try_reserve

John Ericson (Nov 30 2019 at 15:15, on Zulip):

BTW, how does capacity overlow happen without an alloc failure? Shouldn't it be underflow?

Tim Diekmann (Nov 30 2019 at 15:15, on Zulip):

You can still do this, but you get the desired behavior when not using AbortAlloc and dropping the Error = ! bound

Tim Diekmann (Nov 30 2019 at 15:17, on Zulip):

capacity overflow may only occure on 16bit and 32 bit platforms, as only isize capacity is allowed there. If you try to reserve isize::MAX / 4 slots with size_of::<T>() == 8 it will overflow

John Ericson (Nov 30 2019 at 15:18, on Zulip):

Wouldn't that also be an allocation failure?

John Ericson (Nov 30 2019 at 15:18, on Zulip):

err nevermind

John Ericson (Nov 30 2019 at 15:19, on Zulip):

@Tim Diekmann So honestly if what is in alloc-wg landed soon, and the try_* methods were unstable until GAT, I would be perfectly happy

John Ericson (Nov 30 2019 at 15:19, on Zulip):

what do you think about that?

Tim Diekmann (Nov 30 2019 at 15:24, on Zulip):

Wouldn't that also be an allocation failure?

Yes, but currently CapacityOverflow simply panics in RawVec::reserve. In RawVec::try_reserve it returns Err(CapacityAllocError::CapacityOverflow).
Vec::try_reserve also returns Result<(), CapacityAllocError>, but only the CapacityOverflow variant.

Tim Diekmann (Nov 30 2019 at 15:25, on Zulip):

Even if the alloc-wg stuff will be merged upstream, it will take at least a year I guess until anything is stabilized. GATs are not required for the try_* methods, only for other trait methods on collections, but this is another story and probably out of scope for this WG (currently).

John Ericson (Nov 30 2019 at 15:26, on Zulip):

It might be for like clone_in and other multi-allocator ones?

Tim Diekmann (Nov 30 2019 at 15:28, on Zulip):

No, generic trait methods for more collections. CloneIn is possible without GATs, too. However, even if a trait would need GATs I'd also be fine waiting for it. We shouldn't make the API more complicated than necessary, only because an approved language feature isn't completed yet.

John Ericson (Nov 30 2019 at 15:29, on Zulip):

sorry try_clone_in if we wanted to somehow gate that too

Tim Diekmann (Nov 30 2019 at 15:30, on Zulip):

try_clone_in :slight_smile:

John Ericson (Nov 30 2019 at 15:31, on Zulip):

Oh right, that workaround

John Ericson (Nov 30 2019 at 15:31, on Zulip):

And an additional trait could also use it

Tim Diekmann (Nov 30 2019 at 15:31, on Zulip):

master doc: https://timdiekmann.github.io/alloc-wg/alloc_wg/clone/trait.CloneIn.html#tymethod.try_clone_in

Tim Diekmann (Nov 30 2019 at 15:33, on Zulip):

But I'm unsure about the error type there. The error type may should be associated with the CloneIn trait itself, so we can return CollectionAllocErr to retrieve the layout

Tim Diekmann (Nov 30 2019 at 15:33, on Zulip):

This is dropped here

John Ericson (Nov 30 2019 at 15:34, on Zulip):

Ah that

Tim Diekmann (Nov 30 2019 at 15:34, on Zulip):

@John Ericson To finish this topic, do you agree that AbortAlloc makes no sense like this?

John Ericson (Nov 30 2019 at 15:35, on Zulip):

I think it makes sense if you are very used to !, but I'm fine not doing it unless it can be made less surprising for everything that isn't.

John Ericson (Nov 30 2019 at 15:36, on Zulip):

The big battle here is just getting try_ methods on the collections at all

Tim Diekmann (Nov 30 2019 at 15:36, on Zulip):

Yeah, it's a nice thing regarding !, but incompatible with try_*.

John Ericson (Nov 30 2019 at 15:37, on Zulip):

So without the Error = ! bounds, I suspect people will push to drop the associated error type altogether?

Tim Diekmann (Nov 30 2019 at 15:37, on Zulip):

However, Error=! as bound on reserve and other non-try_ methods is just a way to force the user to use try instead.

John Ericson (Nov 30 2019 at 15:38, on Zulip):

Yes, it is nice to pass in a collection to a library that is allocator polymorphic, and force it to do the right thing

Tim Diekmann (Nov 30 2019 at 15:38, on Zulip):

So without the Error = ! bounds, I suspect people will push to drop the associated error type altogether?

Yes, another thing I totally forgot: You would have need to wrap nearly all allocators in AbortAlloc to just ignore the associated error. Just as you mentioned, the ecosystem is split there.

Tim Diekmann (Nov 30 2019 at 15:39, on Zulip):

It's a nice thing, if you want to never panic/abort. But that's only the unusual case.

Tim Diekmann (Nov 30 2019 at 15:39, on Zulip):

Normal programs will never recover from OOM anyway.

John Ericson (Nov 30 2019 at 15:40, on Zulip):

We can still support both ecosystem with the approach on master today

Tim Diekmann (Nov 30 2019 at 15:40, on Zulip):

And the capacity overflow panic is a bug in the program

John Ericson (Nov 30 2019 at 15:40, on Zulip):

Just you are really reliant on checking for master by hand

John Ericson (Nov 30 2019 at 15:40, on Zulip):

But I'll admit that any library should be able to do the foo try_foo duplication with the panic rather than Error = ! bound

John Ericson (Nov 30 2019 at 15:41, on Zulip):

so the same libraries can support both still

John Ericson (Nov 30 2019 at 15:41, on Zulip):

Yeah I am much more sympathetic to capacity overflow always being a regular overflow panic

John Ericson (Nov 30 2019 at 15:42, on Zulip):

and well, if you don't want to panic run a model checker or something

John Ericson (Nov 30 2019 at 15:42, on Zulip):

as you shouldn't sanitize uncontrolled values by catching those errors, since as you say this program really is broken whereas normal allocation failure can be not the program's fault

Tim Diekmann (Nov 30 2019 at 15:43, on Zulip):

Another Idea: I think we are able to support both ecosystems this way: ...

Tim Diekmann (Nov 30 2019 at 15:44, on Zulip):

- Introduce a marker trait Abort
- for reserve and allocating non-try_* methods add this as bound
- Implement Abort on Global and System

Tim Diekmann (Nov 30 2019 at 15:44, on Zulip):

That's basically the inversion of AbortAlloc, right?

Tim Diekmann (Nov 30 2019 at 15:45, on Zulip):

And if you want to be forced to check OOM, you just have to wrap your allocator but don't implement Abort

John Ericson (Nov 30 2019 at 15:45, on Zulip):

:)

Tim Diekmann (Nov 30 2019 at 15:45, on Zulip):

That also removes the very missleading term of "infallible"

Tim Diekmann (Nov 30 2019 at 15:45, on Zulip):

Global isn't infallible, we want to ignore the error

Tim Diekmann (Nov 30 2019 at 15:46, on Zulip):

Abort allows the allocator to fail

John Ericson (Nov 30 2019 at 15:46, on Zulip):

the one thing is it's the collection doing the panicking, not the allocator, right?

John Ericson (Nov 30 2019 at 15:46, on Zulip):

I think you want Abort and AbortAlloc

Tim Diekmann (Nov 30 2019 at 15:47, on Zulip):

Yes, I think so

Hmm, let's see. let me write a few lines of code...

John Ericson (Nov 30 2019 at 15:47, on Zulip):

if you have an A: Abort Then it does a cast to AbortAlloc<A> and calls the method with that?

Tim Diekmann (Nov 30 2019 at 15:49, on Zulip):
trait Abort {}

impl Abort for Global {}

impl<T, A: DeallocRef> Vec<T, A> {
    pub fn reserve(&mut self, additional: usize)
    where
        A: ReallocRef + Abort,
    {
        match self.try_reserve(used_capacity, needed_extra_capacity) {
            Ok(vec) => vec,
            Err(CollectionAllocErr::CapacityOverflow) => capacity_overflow(),
            Err(CollectionAllocErr::AllocError { layout, .. }) => handle_alloc_error(layout.into()),
        }
    }

    pub fn try_reserve(&mut self, additional: usize) -> Result<(), CollectionAllocErr<A>>
    where
        A: ReallocRef,
    {
        self.buf.try_reserve(self.len, additional)
    }
}
Tim Diekmann (Nov 30 2019 at 15:51, on Zulip):
// Don't implement `Abort` on `SaveAlloc`
struct SaveAlloc<A>(A);

// impl `AllocRef` etc. for `SaveAlloc`
John Ericson (Nov 30 2019 at 15:51, on Zulip):

oh nice!

John Ericson (Nov 30 2019 at 15:51, on Zulip):

Yeah that's good, you can make a method that also requires Abort which calls handle_alloc_error

Tim Diekmann (Nov 30 2019 at 15:52, on Zulip):

Yeah, true! nice!

John Ericson (Nov 30 2019 at 15:52, on Zulip):

the A is sort of phantom as it takes an Result<T, A::Error>

John Ericson (Nov 30 2019 at 15:52, on Zulip):

and another for Result<T, CollectionsError<A::Error>>

Tim Diekmann (Nov 30 2019 at 15:53, on Zulip):

Wow, I'm very excited, I think this is really a very good solution :)

John Ericson (Nov 30 2019 at 15:54, on Zulip):

me too! I was worried it would be endless type machinary, but this isn't even that bad!

Tim Diekmann (Nov 30 2019 at 15:54, on Zulip):

It's just a single marker trait. We already got plenty marker traits anyway

John Ericson (Nov 30 2019 at 15:55, on Zulip):

yeah that's not a controversial thing by now

John Ericson (Nov 30 2019 at 15:55, on Zulip):

If you want to delegate to me to write it up, then you can get your time back with me rehashing the old AbortAlloc :)

Tim Diekmann (Nov 30 2019 at 15:56, on Zulip):

I have to go now anyway. If you want to do this, go ahead!

Tim Diekmann (Nov 30 2019 at 15:56, on Zulip):

I'll close your current PR then?

John Ericson (Nov 30 2019 at 15:57, on Zulip):

Yeah

Tim Diekmann (Nov 30 2019 at 15:58, on Zulip):

Hey:

impl<T> Abort for T where T: AllocRef<Error = !> {}
John Ericson (Nov 30 2019 at 15:58, on Zulip):

Yes can do that too, though will still need the separate Global and System ones

Gurwinder Singh (Nov 30 2019 at 15:59, on Zulip):

:+1:

Tim Diekmann (Nov 30 2019 at 16:00, on Zulip):

Yes can do that too, though will still need the separate Global and System ones

Sure, but at least you don't have to implement Abort on any new allocators :)

Tim Diekmann (Nov 30 2019 at 16:04, on Zulip):

Isn't this a good point to keep the associated error type?

John Ericson (Nov 30 2019 at 16:06, on Zulip):

Yes it is!

John Ericson (Nov 30 2019 at 16:07, on Zulip):

@Tim Diekmann OK so first issue is I think we do need AbortAlloc with this, because in general don't have the layout for handle_alloc_error

John Ericson (Nov 30 2019 at 16:07, on Zulip):

this is partially why the boxed ones still use unwrap_unchecked by mistake

John Ericson (Nov 30 2019 at 16:07, on Zulip):

it's a lot less mechanical to fix

John Ericson (Nov 30 2019 at 16:07, on Zulip):

but AbortAlloc can be private

Tim Diekmann (Nov 30 2019 at 16:25, on Zulip):

Yes, this would replace the AbortAlloc thing entirely.

I don't have more time today to head more into this, I'll look more into this tomorrow :)
Just go ahead :)

gnzlbg (Nov 30 2019 at 17:34, on Zulip):

FWIW the FallibleVec would be what Vec is today

gnzlbg (Nov 30 2019 at 17:34, on Zulip):

and Vec would just become a "thin" wrapper over FallibleVec that .unwraps() on every method

gnzlbg (Nov 30 2019 at 17:36, on Zulip):

There wouldn't be any _try methods

gnzlbg (Nov 30 2019 at 17:36, on Zulip):

You would just pick Vec or FallibleVec depending on whether you want an infallible or fallible API

Tim Diekmann (Nov 30 2019 at 17:45, on Zulip):

Yes, but it would introduce many many new structs

John Ericson (Nov 30 2019 at 18:10, on Zulip):

You would just pick Vec or FallibleVec depending on whether you want an infallible or fallible API

@gnzlbg beyond the many new structs, it would be much harder to reuse the implementation

John Ericson (Nov 30 2019 at 18:12, on Zulip):

https://github.com/TimDiekmann/alloc-wg/pull/16 is now up with @Tim Diekmann's Abort trait idea. I had to put in AllocAbort as an implementation detail, but a better method might be changing the error types to be something ~~ (AllocError, NonZeroLayout)

John Ericson (Nov 30 2019 at 18:13, on Zulip):

I say we try to get this one merged with the private AllocAbort, and then make another issue for the error handling, as the temporary AllocAbort is good to make the issue clear.

gnzlbg (Dec 02 2019 at 09:17, on Zulip):

@John Ericson by many new structs you mean, ~5 structs ? (Vec, List, HashMap, BTreeSet, Deque) ?

gnzlbg (Dec 02 2019 at 09:17, on Zulip):

Also, why do you think it would be harder to reuse the implementation ?

gnzlbg (Dec 02 2019 at 09:17, on Zulip):

You can reuse Vec like you do today, and for FallibleVec it would be just the same.

Tim Diekmann (Dec 02 2019 at 14:37, on Zulip):

At least those 5 structs + strings + any other structs that will be associated with an allocator.
Additionally, every api addition has to be added to both structs, which is very error prone. Also, it's a huge documentation bloat. I don't see the advantages of FallibleVec in comparison to the Abort trait or the current solution.

Tim Diekmann (Dec 02 2019 at 14:38, on Zulip):

Tracking Issue for structs which needs an allocator
This would introduce at least 15(!) new structs for a rarely used API.

gnzlbg (Dec 02 2019 at 14:43, on Zulip):

Yeah those 15 types would be pretty much it

gnzlbg (Dec 02 2019 at 14:44, on Zulip):

Many API additions already need to be added to multiple types, e.g., to Vec and RawVec, particularly when they interact with allocators

gnzlbg (Dec 02 2019 at 14:44, on Zulip):

notice that this duplication of the API is only required for functions that could allocate

gnzlbg (Dec 02 2019 at 14:44, on Zulip):

so maybe there is a better way to land it than duplicating all structs

gnzlbg (Dec 02 2019 at 14:45, on Zulip):

I doubt that though, because while it is possible to write an infallible API on top of a fallible one, e.g., using a trait, you can't write a fallible API on top of an infallible one

gnzlbg (Dec 02 2019 at 14:46, on Zulip):

also, with a stable allocator interface, if somebody wants a fallible vector, they can just use one from crates.io

gnzlbg (Dec 02 2019 at 14:47, on Zulip):

doing that this way literally requires duplicating most of the code, but it can be done

John Ericson (Dec 02 2019 at 15:00, on Zulip):

@gnzlbg I'm confused. There's now way to write arbitrary vec-polymorphic code without higher kinded types, so having multiple vecs seems like a disaster? What problems do you have with https://github.com/TimDiekmann/alloc-wg/pull/16 ?

John Ericson (Dec 02 2019 at 15:03, on Zulip):

Also confused about the crates.io bit. I agree it would be good if more collections came from crates.io—std can even have private deps and re-export. But this is totally separate from allocator stuff? I.e. we should "pull a hash brown" for everything else anyways?

gnzlbg (Dec 02 2019 at 15:18, on Zulip):

What problems do you have with https://github.com/TimDiekmann/alloc-wg/pull/16 ?

If I'm writing a library that should propagate all alloaction errors to users, and my library uses an API like Vec::extend, how do I propagate the error ?

gnzlbg (Dec 02 2019 at 15:21, on Zulip):

There's now way to write arbitrary vec-polymorphic code without higher kinded types, so having multiple vecs seems like a disaster?

Why would you need that ?

gnzlbg (Dec 02 2019 at 15:21, on Zulip):

If you have an API that should accept / return multiple vector types, like Vec, SmallVec, or ArrayVec, then you can write a VecLike trait, and implement it for those types.

gnzlbg (Dec 02 2019 at 15:22, on Zulip):

I don't see how another kind of Vec makes this problem worse.

John Ericson (Dec 02 2019 at 15:33, on Zulip):

If I'm writing a library that should propagate all alloaction errors to users, and my library uses an API like Vec::extend, how do I propagate the allocation error to a user such that it can recover ?

You write a mylib::try_method with Vec::try_extend, and then in the same manner make your mylib::method in terms of mylib::try_method.

John Ericson (Dec 02 2019 at 15:34, on Zulip):

I don't see how another kind of Vec makes this problem worse.

I don't see how another kind of Vec makes this problem worse. It doesn't make the problem worse, but it's already bad. There's no good way to write VecLike today, and generic associated types only make that slightly better.

Tim Diekmann (Dec 02 2019 at 17:05, on Zulip):

I reviewed your PR @John Ericson. Thanks again for implementing it :)

gnzlbg (Dec 03 2019 at 15:41, on Zulip):

@John Ericson

You write a mylib::try_method with Vec::try_extend

There is no try_extend method.

gnzlbg (Dec 03 2019 at 15:41, on Zulip):

And that approach would require duplicating most of the API of all collections

gnzlbg (Dec 03 2019 at 15:42, on Zulip):

I don't see how that is better than having two types providing the different APIs, and without the try_ suffixes

gnzlbg (Dec 03 2019 at 15:43, on Zulip):

It doesn't make the problem worse, but it's already bad. There's no good way to write VecLike today, and generic associated types only make that slightly better.

I see that as an orthogonal issue. The argument that we should not do this because it would make something that's already impossible to do not harder isn't very strong.

Tim Diekmann (Dec 09 2019 at 11:56, on Zulip):

I actually tried out a FallibleVec. Besides my previous concerns, I also went into a no-go: You cannot trivially switch between a fallible and an aborting allocation, you have to know beforehand, if all OOMs aborts or return a Result.

Tim Diekmann (Dec 09 2019 at 11:59, on Zulip):

With the Abort marker trait, you can chose, if you allow aborting: you have to implement Abort for your allocator. If you don't implement it, the collection cannot abort. But if you do, you can chose, if your current allocation is allowed to abort or not.

gnzlbg (Dec 13 2019 at 14:03, on Zulip):

@Tim Diekmann i'm not sure i follow

gnzlbg (Dec 13 2019 at 14:05, on Zulip):

With a FallibleVec if a call to an allocation function returns ptr::null() then you just propagate Result::Err up to the caller. The caller is then in charge of , if that Result is an error, calling handle_alloc_error

gnzlbg (Dec 13 2019 at 14:05, on Zulip):

e.g. that;s what a Vec<T> wrapper built on top of FallibleVec<T> would do

gnzlbg (Dec 13 2019 at 14:05, on Zulip):

(this is in the current world where there is no AllocRef trait)

Tim Diekmann (Dec 13 2019 at 14:06, on Zulip):

Which vector would you use, when you don't care about OOM in one place, but catch OOMs in another place?

gnzlbg (Dec 13 2019 at 14:08, on Zulip):

When you don't care about OOM, you use Vec<T>, which calls handle_alloc_error on OOM

gnzlbg (Dec 13 2019 at 14:08, on Zulip):

When you care about OOM you use FallibleVec which returns a Result::Err on OOM

Tim Diekmann (Dec 13 2019 at 14:09, on Zulip):

Yeah, but you need two dedicated structs here
You always have to know beforehand if you want to catch OOM or not

gnzlbg (Dec 13 2019 at 14:09, on Zulip):

The layout of both vector types is identical, so if you need to handle OOM in certain situations, and not in others

gnzlbg (Dec 13 2019 at 14:09, on Zulip):

you can just go from Vec -> FallibleVec and from FallibleVec->Vec

gnzlbg (Dec 13 2019 at 14:09, on Zulip):

its a noop

gnzlbg (Dec 13 2019 at 14:10, on Zulip):

well sure, that's the point, allowing users that know before hand what they want what to do

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

Hmm, That's a pretty bad design IMO, as you probably want to store the vector in a struct

gnzlbg (Dec 13 2019 at 14:10, on Zulip):

if you need a library that supports both, you can always just use FallibleVec

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

And call handle_alloc_error manually?

gnzlbg (Dec 13 2019 at 14:11, on Zulip):

Or propagate the errors to the API of your library

gnzlbg (Dec 13 2019 at 14:11, on Zulip):

if the final binary doesn't care about OOM, they can just choose a global allocator that does that for you to fail fast

gnzlbg (Dec 13 2019 at 14:12, on Zulip):

Hmm, That's a pretty bad design IMO, as you probably want to store the vector in a struct

Maybe, you can just swap the Vec out, since that is a nop as well

gnzlbg (Dec 13 2019 at 14:12, on Zulip):

But i'm open to any better design that satisfies all the constraints

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

The current design supports all constraints :D

gnzlbg (Dec 13 2019 at 14:13, on Zulip):

Then I didn't understood it, because Vec::push calls handle_alloc_err

gnzlbg (Dec 13 2019 at 14:13, on Zulip):

and that's a constraint that it doesn't satisfy AFAICT

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

Yeah, this is just like the current implementation does

gnzlbg (Dec 13 2019 at 14:14, on Zulip):

Yeah, so I think a constraint is that every function that can trigger an OOM in any std collection should be recoverable

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

If you want to get the alloc-error on push you have to call try_push

gnzlbg (Dec 13 2019 at 14:14, on Zulip):

And I'd prefer not to have to learn which subset of these APIs are available via try_ methods

gnzlbg (Dec 13 2019 at 14:14, on Zulip):

but instead just be able to call the normal method and get a result

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

That's a good point

gnzlbg (Dec 13 2019 at 14:15, on Zulip):

if you don't want to move the vector out of a struct, you can always have a Vec::fallible_mut_ref(&mut self) -> &mut FallibleVec

gnzlbg (Dec 13 2019 at 14:15, on Zulip):

that provides fallible operations

gnzlbg (Dec 13 2019 at 14:16, on Zulip):

because they have the exact same layout, you can safely transmute a &Vec/&mut Vec into a &FallibleVec/&mut FallibleVec in place

Tim Diekmann (Dec 13 2019 at 14:16, on Zulip):

But then Vec and FallibleVec are not interchangable

Tim Diekmann (Dec 13 2019 at 14:16, on Zulip):

as the signatures of the functions differ

gnzlbg (Dec 13 2019 at 14:16, on Zulip):

that's the goal?

gnzlbg (Dec 13 2019 at 14:16, on Zulip):

with one API, you don't get Results returned, because you don't care

Tim Diekmann (Dec 13 2019 at 14:16, on Zulip):

Yeah, but then it's pretty unflexible

gnzlbg (Dec 13 2019 at 14:16, on Zulip):

and with the other, you get Results returned, because you do care

gnzlbg (Dec 13 2019 at 14:16, on Zulip):

and you can choose whether you care or not

Tim Diekmann (Dec 13 2019 at 14:17, on Zulip):

If you do care about OOM, you can just use an allocator, which does not support aborting

gnzlbg (Dec 13 2019 at 14:17, on Zulip):

Whether to abort on OOM or not is not up to the allocator though

Tim Diekmann (Dec 13 2019 at 14:17, on Zulip):

Then the compiler won't let you call push

gnzlbg (Dec 13 2019 at 14:17, on Zulip):

Then you have two types Vec<T, A>, and Vec<T, AbortA> and these types are not interchangeable AFAICT

Tim Diekmann (Dec 13 2019 at 14:17, on Zulip):

Maybe we need a third opinion here :)

Tim Diekmann (Dec 13 2019 at 14:18, on Zulip):

But you can call any methods on Vec<T, AbortA>

gnzlbg (Dec 13 2019 at 14:18, on Zulip):

Your option of not allowing calling Vec::push still has the drawback of requiring to duplicate the whole API with try_ methods.

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

With your option you has to duplicate the whole vector, even the non-allocating functions

gnzlbg (Dec 13 2019 at 14:19, on Zulip):

Sure, but users don't have to learn any new APIs

gnzlbg (Dec 13 2019 at 14:19, on Zulip):

Duplicating the whole vector is cheap

John Ericson (Dec 13 2019 at 14:19, on Zulip):

Gbzlbg we can do something about the try thing long term

gnzlbg (Dec 13 2019 at 14:20, on Zulip):

As in, its just a simple wrapper type struct Vec<T, A>(FallibleVec<T, A>); that exposes the same API as FallibleVec, but calling handle_alloc_error instead of returning Result

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

But it's a lot harder to merge two types that have impls in the wild long term

gnzlbg (Dec 13 2019 at 14:21, on Zulip):

I think having the collections be infallible by default, but offering a way to get a fallible handle, is much more simpler for users, than duplicating the whole API with different function names.

gnzlbg (Dec 13 2019 at 14:21, on Zulip):

its also more amenable for macro!s, since they don't have to, e.g., concatenate try_ to identifiers

gnzlbg (Dec 13 2019 at 14:22, on Zulip):

although that would be another option, e.g., fallible!(foo.push()) gets expanded to foo.try_push()

John Ericson (Dec 13 2019 at 14:22, on Zulip):

With the current way it's really easy to make foo and try_foo methods downstream

John Ericson (Dec 13 2019 at 14:22, on Zulip):

That's a big requirement for me

John Ericson (Dec 13 2019 at 14:23, on Zulip):

So we can actually have an ecosystem that supports both

John Ericson (Dec 13 2019 at 14:24, on Zulip):

Two types create so much more friction

John Ericson (Dec 13 2019 at 14:24, on Zulip):

I don't think anyone is going to bother supporting both

John Ericson (Dec 13 2019 at 14:24, on Zulip):

And the whole thing is dead in the water

gnzlbg (Dec 13 2019 at 14:27, on Zulip):

I don't understand the difference

gnzlbg (Dec 13 2019 at 14:27, on Zulip):

Maybe you can try to explain it in a different way?

gnzlbg (Dec 13 2019 at 14:28, on Zulip):

With my proposal, when you implement a collection, you make all methods fallible, and then generate a wrapper that just calls handle_alloc_error, you can probably auto-generate the infallible wrapper with a proc macro derive.

gnzlbg (Dec 13 2019 at 14:29, on Zulip):

Since you can go from one type to the other, you don't have to support two types AFAICT.

gnzlbg (Dec 13 2019 at 14:29, on Zulip):

Or at least, no more than having to support different kinds of collections.

gnzlbg (Dec 13 2019 at 14:30, on Zulip):

I mean, if you have something like serde serialize, it is trivial to derive two implementation for the two vectors, such that one propagates oom errors and the otherone doesn't

gnzlbg (Dec 13 2019 at 14:31, on Zulip):

If you only have a single Vec type with Vec::try_ methods, you get a single Deserialize implementation, that's not going to use the Vec::try_ methods.

gnzlbg (Dec 13 2019 at 14:31, on Zulip):

and if you want to recover from a deserialization that fails due to OOM, you need to write a vector wrapper anyways

John Ericson (Dec 13 2019 at 14:33, on Zulip):

Say your type has public constructors and uses Vec

John Ericson (Dec 13 2019 at 14:33, on Zulip):

Say your type has methods that use Vec

John Ericson (Dec 13 2019 at 14:33, on Zulip):

Now you can't use macro

gnzlbg (Dec 13 2019 at 14:35, on Zulip):

?

gnzlbg (Dec 13 2019 at 14:36, on Zulip):

If you have

let v: FallibleVec<T>;
fn foo(a: Vec<T>) -> Vec<T>;
// you just:
let v: FallibleVec<T> = foo(v.infallible()).fallible();
gnzlbg (Dec 13 2019 at 14:37, on Zulip):

where FallibleVec::infallible(self) -> Vec and Vec::fallible(self) -> FallibleVec

John Ericson (Dec 13 2019 at 14:40, on Zulip):

I don't think the macro can do that for arbitrary signatures with Vec

John Ericson (Dec 13 2019 at 14:40, on Zulip):

Especially if the thing parameterized with a Vec is abstract

John Ericson (Dec 13 2019 at 14:42, on Zulip):

You need something like Haskell's Coercible to do this

John Ericson (Dec 13 2019 at 14:42, on Zulip):

Full stop

gnzlbg (Dec 13 2019 at 14:54, on Zulip):

I'm not sure what you mean with "the macro can do that"

gnzlbg (Dec 13 2019 at 14:56, on Zulip):

They are different types, if you need to go from one to another, you just call a conversion function.

gnzlbg (Dec 13 2019 at 14:56, on Zulip):

Into::into would do

gnzlbg (Dec 13 2019 at 14:56, on Zulip):
let v: FallibleVec<T> = foo(v.into()).into();
Lokathor (Dec 13 2019 at 19:37, on Zulip):

I would favor always writing try_foo first and then also having foo that simply does try_foo(arg).unwrap() be available for however much of the stuff you think people would want to unwrap all the time (perhaps even 100% of it)

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

actually in the case of this it would be more like try_foo(arg).or_else(|_|handle_alloc_error())

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

Default to infallible is just not robust in the same way that unwrapping every Result and Option isn't very robust

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

I'm not sure what you are arguing for or against

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

Are you suggesting that every liballoc method that alloactes should have a try_ counterpart ?

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

Like Vec::try_extend ?

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

I find that handling allocation errors is already uncomfortable enough, adding also the burden of writing try_{everything} makes this worse, and it also makes it easier for "bugs" to be introduced by accidentally using a non-try_ method when you shouldn't.

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

Currently you don't handle allocation errors in collection at all (besides handle_alloc_error, which defaults to abort and I never have seen anyone using it).
Why this would it makes easier for bugs? It's not possible to use non-try methods on allocators, which are not allowed to abort

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

Vec::try_reserve exists

gnzlbg (Dec 19 2019 at 13:00, on Zulip):

If I have code that must not fail on OOM, and I use try_ methods everywhere, and then somebody accidentally slips a non-try_ call somewhere, that's a bug, potentially a very big one.

gnzlbg (Dec 19 2019 at 13:00, on Zulip):

On the level of a security vulnerability

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

Then you use an allocator, which does not implement Abort

gnzlbg (Dec 19 2019 at 13:01, on Zulip):

And then the non-try_ methods are not available ?

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

yup

gnzlbg (Dec 19 2019 at 13:01, on Zulip):

So why do I have to type more ?

gnzlbg (Dec 19 2019 at 13:01, on Zulip):

And why do I need a different allocator ?

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

Because you may want to use try_ on collection, which are allowed to abort

gnzlbg (Dec 19 2019 at 13:02, on Zulip):

How is handling OOM an allocator issue ?

gnzlbg (Dec 19 2019 at 13:02, on Zulip):

An allocator reports whether OOM happen, but it isn't its job to handle it

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

I see, that using try_ isn't the best thing. but it's still much better than duplicating every collection

gnzlbg (Dec 19 2019 at 13:03, on Zulip):

You are not duplicating every collection

gnzlbg (Dec 19 2019 at 13:03, on Zulip):

That claim is super misleading

gnzlbg (Dec 19 2019 at 13:03, on Zulip):

You are providing a type safe thin wrapper that expresses an invariant at the type level

gnzlbg (Dec 19 2019 at 13:03, on Zulip):

You are not duplicating all the code involved

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

However you have two nearly same collection

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

Additionally, the allocator isn't envolved in OOM handling. Abort is just a marker trait

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

No, you have two different _types_ exposing two different ways of modifying a collections owned data using a single API

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

So if I have an allocator, when should I implement Abort for it ?

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

Abort is implemented for Global, System and every allocator, which are infallible

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

This isn't true

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

Since Global and System can return a ptr::null on allocations

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

and that means that the allocation failed

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

Abort is implemented for Global, System and every allocator, which are infallible

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

Sure but that isn't true, since Global and System are fallible

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

today

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

You marking allocators, which are allowed to abort

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

Abort does not mean, the allocator is infallible

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

which are infallible

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

So what does Abort mean ?

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

Otherwise it would be named Infallible

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

Marker trait to indicate that the allocator is allowed to abort on OOM.

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

Allowed but not required

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

right?

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

Yes

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

So the allocator can still return a null pointer

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

yes

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

the collection handles the abortion then

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

And allocators that do not have the Abort marker trait can abort on OOM, right ?

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

It's a marker trait regarding handling the OOM, not regarding allocating

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

Can you be more clearer ?

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

Seems like you are suggesting that implementing Abort for allocators means that the allocator might abort on OOM, which is something that all allocators are allowed to do any ways, but that this trait should hint users of the allocator, that they should abort if the allocator returns an error, which is something that users of the allocators can already do without the trait.

gnzlbg (Dec 19 2019 at 13:11, on Zulip):

If an allocator without Abort returns a null pointer, handle_alloc_error still can be called

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

Take the Vec method reserve, it's bound on Abort:

pub fn reserve(&mut self, additional: usize) where ReallocRef + Abort;

If you chose an allocator, which implements Abort you may call Vec::reserve. But you are still able to call `try_reserve:

pub fn try_reserve(&mut self, additional: usize) -> Result<(), CollectionAllocErr<A>> where A: ReallocRef;
Tim Diekmann (Dec 19 2019 at 13:13, on Zulip):

That's the whole thing of the Abort trait

gnzlbg (Dec 19 2019 at 13:13, on Zulip):

It feels very weird to use a trait on allocators to customize collections behavior

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

That's probably a good point

gnzlbg (Dec 19 2019 at 13:14, on Zulip):

Like for jemalloc, should I implement abort or not ?

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

Depends on your needs

gnzlbg (Dec 19 2019 at 13:14, on Zulip):

that's for the user of jemalloc to decide on every vector they use

gnzlbg (Dec 19 2019 at 13:14, on Zulip):

not for the allocator to decide

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

But I see the point

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

What you want to express is whether a particular vector should abort on OOM, or return an error

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

and that's orthogonal to the allocator used

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

Right now, Vec::reserve just calls try_reserve and unwraps.

gnzlbg (Dec 19 2019 at 13:16, on Zulip):

I'd rather just express the property as part of the vector type somehow

gnzlbg (Dec 19 2019 at 13:16, on Zulip):

When you take Jemalloc, and JemallocAbort (like jemalloc, but implementing Abort), and instantiate Vec<T, Jemalloc> and Vec<T, JemallocAbort> you are duplicating the collection

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

It's probably better to express it on the collection, but not in the way of have two very similar collection of each type in std

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

If we would just have collection traits...

gnzlbg (Dec 19 2019 at 13:17, on Zulip):

you are duplicating the collection

As in, this will instantiate the collection twice.

gnzlbg (Dec 19 2019 at 13:18, on Zulip):

Why would you need collection traits?

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

Then you could implement a generic wrapper around collections

gnzlbg (Dec 19 2019 at 13:18, on Zulip):

If you had a VecLike trait, that has a VecLike::reserve -> ??? method, what would the ??? be ?

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

well, true^^

gnzlbg (Dec 19 2019 at 13:19, on Zulip):

I think the FallibleVec + Vec solution is straightforward

gnzlbg (Dec 19 2019 at 13:19, on Zulip):

Sure, you need to write the same API twice, but that's straightforward to do, and to understand

gnzlbg (Dec 19 2019 at 13:19, on Zulip):

And well you need to implement Vec on top of FallibleVec, but that's also kind of trivial

gnzlbg (Dec 19 2019 at 13:20, on Zulip):

It is more work than adding try_ methods, cause you need to implement the traits for the vector, etc.

gnzlbg (Dec 19 2019 at 13:20, on Zulip):

But from a user of the standard library, both APIs are the same, there is only one API to learn, etc.

gnzlbg (Dec 19 2019 at 13:21, on Zulip):

An alternative might be a Vec<T, A, const ErrorOnOOM: bool = true> kind of thing

gnzlbg (Dec 19 2019 at 13:22, on Zulip):

Where we change the return types of all methods depending on the value of the bool..

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

I like the const generics approach, but I wouldn't use a boolean here. An enum expresses it better, like

enum OomBehavior {
    Abort,
    Error,
}
gnzlbg (Dec 19 2019 at 13:24, on Zulip):
struct Vec<T, A, const ErrorOnOOM = true>(...);
impl<T, A> Vec<T, A, true> {
     fn reserve(...) -> () { handle_alloc_error... }
}
impl<T, A> Vec<T, A, false> {
     fn reserve(...) -> Result<....> { }
}
gnzlbg (Dec 19 2019 at 13:24, on Zulip):

@Tim Diekmann the enum sounds like a better idea

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

I still don't know, if changing the return type is a good idea though

gnzlbg (Dec 19 2019 at 13:26, on Zulip):

I suppose we don't even need const generics

trait ErrorBehavior {}
struct Error;
struct Abort;
impl ErrorBehavior for Error {}
impl ErrorBehavior for Abort {}

struct Vec<T, A, E: ErrorBehavior = Abort>(...);
impl<T, A> Vec<T, A, Abort> {
      fn reserve() -> () {}
}
impl<T, A> Vec<T, A, Error> {
      fn reserve() -> Result<(), ...> {}
}
Tim Diekmann (Dec 19 2019 at 13:27, on Zulip):

Okay, but I'd go with this approach:

gnzlbg (Dec 19 2019 at 13:27, on Zulip):

We can't change the return type for current behavior (would be a backward incompat change), and we want to be able to use Result for the non-standard case, so i'm not sure how else to handle it.

Tim Diekmann (Dec 19 2019 at 13:28, on Zulip):
struct Vec<T, A, E = Abort>(...);
impl<T, A, E> Vec<T, A, E> {
      fn try_reserve() -> Result<(), ...>;
}
impl<T, A> Vec<T, A, Abort> {
      fn reserve() -> ();
}
Tim Diekmann (Dec 19 2019 at 13:29, on Zulip):

However, const generics is more clear. The trait approach is a workaround

gnzlbg (Dec 19 2019 at 13:32, on Zulip):

Sure, I agree.

gnzlbg (Dec 19 2019 at 13:33, on Zulip):

For prototyping purposes, we can get the trait approach working today. The const generics part, I'm not 100% sure if this already works

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

With my last approach, Error is a superset of Abort: when using Abort, you have more methods which can be used

gnzlbg (Dec 19 2019 at 13:37, on Zulip):

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=99631852589832f4d8214729ab418803

gnzlbg (Dec 19 2019 at 13:37, on Zulip):

This works!

gnzlbg (Dec 19 2019 at 13:37, on Zulip):

@Tim Diekmann yes, I don't know how I feel about that

gnzlbg (Dec 19 2019 at 13:38, on Zulip):

I understand the idea behind the super set, i don't know if that's a better API than just choosing whether all methods should abort or report error

gnzlbg (Dec 19 2019 at 13:38, on Zulip):

Typically, it is not that you only care for a single operation, which is what your approach allows

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

Yes, the const generic approach works, but it's not possible to pass a default argument.

gnzlbg (Dec 19 2019 at 13:39, on Zulip):

Not yet

gnzlbg (Dec 19 2019 at 13:39, on Zulip):

The feature isn't finished

gnzlbg (Dec 19 2019 at 13:40, on Zulip):

Is it possible to do so for types on stable rust ?

gnzlbg (Dec 19 2019 at 13:40, on Zulip):

IIRC that's a nightly only feature

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

Yes, as you suggested, a trait should fulfill the needs until landing

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

I think it is. At least, array implementations uses const generics

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

I understand the idea behind the super set, i don't know if that's a better API than just choosing whether all methods should abort or report error

I think it's more understandable, than changing the API

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

IMO it feels unnaturally, when a return type changes with another parameter

gnzlbg (Dec 19 2019 at 13:42, on Zulip):

Which is why I think I still believe that making this two types might still be better

gnzlbg (Dec 19 2019 at 13:42, on Zulip):

But I think it feels unnatural to me to have a different method for it

gnzlbg (Dec 19 2019 at 13:43, on Zulip):

I'm already expressing that I want to handle errors manually at the type level

gnzlbg (Dec 19 2019 at 13:43, on Zulip):

why do i then need to also use completely different method names ?

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

Fair point

The parameter type has still a huge advange: Only the allocating functions has to be duplicated

gnzlbg (Dec 19 2019 at 13:44, on Zulip):

Yes, that's a pretty big advantage

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

Converting between Vec<_, _, Abort> and Vec<_, _, Error> is a noop. Do you have suitable method names?

gnzlbg (Dec 19 2019 at 13:45, on Zulip):

One probably minor disadvantage is the order of the type parameters, if you want to change how the error type is handled, you need to specify an allocator

gnzlbg (Dec 19 2019 at 13:45, on Zulip):

And if we were to change the order of the type parameters, then to specify a different allocator you'd need to specify how errors are handled

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

Yes, but this also applies to HashMap and the hasher

gnzlbg (Dec 19 2019 at 13:46, on Zulip):

yes, I don't think this is a major issue, nor a problem worth solving

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

It's a problem I already didn't like in C++

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

But, yes, minor one

gnzlbg (Dec 19 2019 at 13:46, on Zulip):

Yes, but the solution would be "named type parameters", and that has other problems

gnzlbg (Dec 19 2019 at 13:46, on Zulip):

Either way, if we ever get a general solution to this problem, the Vec API would automatically benefit from it

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

Let's just not focus on this for now

gnzlbg (Dec 19 2019 at 13:47, on Zulip):

So I think I like this approach more than the Abort approach, unless I'm missing more details about how Abort is supposed to work

gnzlbg (Dec 19 2019 at 13:48, on Zulip):

Other WGs have "summary documents", so maybe we could have a hackmd or similar with a summary of the different approaches, and pros and con, that we can keep updated ?

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

Our summary is alloc-wg I guess :slight_smile:

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

So I think I like this approach more than the Abort approach, unless I'm missing more details about how Abort is supposed to work

No, I think that's it

gnzlbg (Dec 19 2019 at 13:50, on Zulip):

The main reason I think I prefer this approach is the separation of concerns

gnzlbg (Dec 19 2019 at 13:50, on Zulip):

Allocators should just need to worry about allocating and deallocating memory, turns out this is already hard enough.

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

to summarize our todays discussion:

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

Regarding the return type:

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

Yes, you are right, we are already expressing "I want to handle errors on this collection, give me the result"

gnzlbg (Dec 19 2019 at 13:52, on Zulip):

the OomBevahior approach is better, than implementing two distinct types, as most of the API isn't affected by allocators anyway

It's better from the implementation point-of-view. It is ok-ish from the user point-of-view, except when they want to change the error behavior, and this requires them to also pass an allocator, but that's a minor inconvenience. The FallibleVec + Vec approach requires duplicating the whole API, but one API reuses the other.

gnzlbg (Dec 19 2019 at 13:53, on Zulip):

One thing I'm not sure is, how to re-use Vec::try_reserve (Or the Vec::reserve(...) -> Result version, from the infallible one with this approach.

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

try_reserve isn't stabilized yet. I don't think I get your point

gnzlbg (Dec 19 2019 at 13:56, on Zulip):

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=c18da87e5fc8691e18362451a4a59e05

gnzlbg (Dec 19 2019 at 13:56, on Zulip):

THis works

gnzlbg (Dec 19 2019 at 13:56, on Zulip):

I meant, that when implementing the Vec<T, A, Abort>::reserve(&mut self, N) -> () { .... } method, we want to call the Vec<T, A, Error>::reserve(...) -> Result method, and "unwrap" (or call handle_alloc_error)

gnzlbg (Dec 19 2019 at 13:57, on Zulip):

To reuse its implementation

gnzlbg (Dec 19 2019 at 13:58, on Zulip):

This "solves" it, but it is not guaranteed to work:

impl<T, A> Vec<T, A, {OnOomError::Abort}> {
      fn fallible(&mut self) -> &mut Vec<T, A, {OnOomError::Error}> {
          unsafe { std::mem::transmute(self) }
      }
}
Tim Diekmann (Dec 19 2019 at 13:58, on Zulip):

Ah yes, your playground would have been my proposal there

gnzlbg (Dec 19 2019 at 13:58, on Zulip):

The problem is that the layout of the two repr(Rust) types isn't necessarily the same

gnzlbg (Dec 19 2019 at 13:59, on Zulip):

so the &mut Vec<.., Abort> -> &mut Vec<..., Error> cast might be unsound

gnzlbg (Dec 19 2019 at 14:00, on Zulip):

The transmute is only safe if the layouts are the same

gnzlbg (Dec 19 2019 at 14:00, on Zulip):

Ahhhhh! wait, i think there is a clause somewhere that says that 1-ZSTs do not affect layout

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

We have the RawVec struct underneath anyway

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

You could just call try_reserve and reserve on RawVec then

gnzlbg (Dec 19 2019 at 14:00, on Zulip):

(deleted)

gnzlbg (Dec 19 2019 at 14:01, on Zulip):

https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/structs-and-tuples.md#structs-with-1-zst-fields

gnzlbg (Dec 19 2019 at 14:01, on Zulip):

For the purposes of struct layout 1-ZST fields are ignored.

gnzlbg (Dec 19 2019 at 14:01, on Zulip):

PhantomData<T> is a 1-ZST

gnzlbg (Dec 19 2019 at 14:02, on Zulip):

So this might be sound

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

You don't have PhantomData<T> in the real Vec implementation

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

Every allocation is backed by RawVec

gnzlbg (Dec 19 2019 at 14:02, on Zulip):

We would need to add it for the OnOomError type parameter

gnzlbg (Dec 19 2019 at 14:02, on Zulip):

Because otherwise the generic parameter is unused

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

You just forward the error type from the Alloc implementation

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

No, I meant for the OnOomError type parameter

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

Ah, true

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

that's not passed to RawVec, or is used anywhere

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

So we need to "use" it

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

Thought your PhantomData<T> denotes the value type

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

Yeah sorry, that was a confusing way to put it

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

However, we don't have to worry about transmuting, as RawVec already has the logic implemented

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

Yeah true, but not for all methods

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

That is, for reserve, sure, we can call RawVec, but for other methods like extend, insert, resize, etc. it might make sense to re-use some of the logic of these methods

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

is Foo(T, U) guaranteed to be the same as Bar(T, U)?

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

https://rust-lang.zulipchat.com/#narrow/stream/136281-t-lang.2Fwg-unsafe-code-guidelines/topic/Guarantees.20about.20layout.20of.20generic.20aggregates/near/183847831

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

I've asked this in the UCGs, but in general no, that's not guaranteed.

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

E.g. rustc is allowed to re-order the fields of Foo and Bar differently, e.g., using PGO data, to put the "hottest" field at the front, and improve cache usage and performance

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

If you want more layout guarantees, you need to use, e.g., repr(C)

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

It would still be possible to implement the logic on a dedicated type

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

Do you have a proper name for converting between those two types? Or should we just use Into, AsRef, and AsMut?

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

We could do both.

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

We can provide, e.g., into() to convert one into the other, taking ownership.

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

But we also could provide a Vec::fallible(&mut self) ->&mut FallibleVec method to allow users to perform fallible operations in a scope, without having to call into twice (to go from Vec->FallibleVec->Vec), or without having to move the vector out of a struct, etc.

Last update: Jul 02 2020 at 19:55UTC