Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Add more support for fallible allocations in Vec #3271

Closed

Conversation

dpaoliello
Copy link
Contributor

Vec has many methods that may allocate memory (to expand the underlying storage, or shrink it down). Currently each of these method rely on "infallible" allocation, that is any failure to allocate will call the global OOM handler, which will (typically) panic. Even if the global OOM handler does not panic, the return type of these method don't provide a way to indicate the failure.

Currently Vec does have a try_reserve method that uses "fallible" allocation: if try_reserve attempts to allocate, and that allocation fails, then the return value of try_reserve will indicate that there was a failure, and the Vec is left unchanged (i.e., in a valid, usable state). We propose adding more of these try_ methods to Vec, specifically for any method that can allocate. However, unlike most RFCs, we are not suggesting that this proposal is the best among many alternatives (in fact we know that adding more try_ methods is undesirable in the long term), instead we are suggesting this as a way forward to unblock important work (see the "Motivations" section below) while we explore other alternatives.

Rendered

PR to add new methods

@ehuss ehuss added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label May 24, 2022
text/0000-vec-fallible-allocation.md Outdated Show resolved Hide resolved
text/0000-vec-fallible-allocation.md Outdated Show resolved Hide resolved
text/0000-vec-fallible-allocation.md Show resolved Hide resolved
@programmerjake
Copy link
Member

note that there are proposals (such as the Storages proposal) that change Vec to use something other than A: Allocator, so maybe there should be a comment to that effect somewhere so all of these don't get accidentally stabilized with stable A: Allocator arguments.

@ojeda
Copy link

ojeda commented May 24, 2022

Out of the given alternatives, it seems to me the one proposed by the RFC is the simplest (only new methods are added, no new compiler features needed, etc.) and the most flexible too (allows to mix fallible/infallible paths, can be used in all environments, etc.).

The "decision point for developers" drawback can be minimized by adding some documentation-level feature, such as grouping of methods (potentially collapsed by default) or having "alternative methods" of others, etc. This also makes it a future, orthogonal improvement, rather than an alternative ("Add a trait for the fallible allocation functions") that affects the design.

@jdahlstrom
Copy link

Another possible alternative: a light-weight fallible wrapper or view type that you could use at any point to opt-in to fallible allocation but still retain compatibility with APIs that take vanilla Vecs.

However, it also occurs to me that a fallible-alloc Vec may not want to be compatible with APIs that take infallible-alloc Vecs by move or &mut because said API might do things that cause allocations! APIs that take Vecs by shared ref would be fine, but those should be rare because taking a slice is the idiomatic way.

@Ericson2314
Copy link
Contributor

Another possible alternative: a light-weight fallible wrapper or view type that you could use at any point to opt-in to fallible allocation but still retain compatibility with APIs that take vanilla Vecs.

That would be fine with me. Wrapping &Vec and &mut Vec does not have the downsides that wrapping Vec itself does.

However, it also occurs to me that a fallible-alloc Vec may not want to be compatible with APIs that take infallible-alloc Vecs by move or &mut because said API might do things that cause allocations! APIs that take Vecs by shared ref would be fine, but those should be rare because taking a slice is the idiomatic way.

That is what #[cfg(not(no_global_oom_handling))] is for, along with the Cargo feature that could replace it (#3140).

It's very subtle to both allow enforcing a policy and avoid causing unnecessary ecosystem forks --- those goals a dangerously close to being contradictory.

With the aforementioned mechanisms, we move the goal posts slightly. Types prioritize not splitting the ecosystem, while cfg mechanism prioritize enforcing a policy. We've floated other solutions (e.g. associated error types on allocators), but the current method strikes me as easiest to wrap ones head around, with the type-level vs cfg-level division of labor.

@dpaoliello
Copy link
Contributor Author

@m-ou-se Any comments on this RFC? I've like to get the Vec::try_ methods merged in, even if they never stabilize and are subsequently removed.

@jdahlstrom
Copy link

jdahlstrom commented Jun 1, 2022

If these methods are added to Vec, it should be ensured that they do not clutter the docs and eg. IDE completions. Unfortunately, stddocs display unstable items in stable docs and there's no setting to toggle their visibility. If this is not feasible (without making them all #[doc(hidden)]) then IMO this is a large drawback of the proposal and should be mentioned in the RFC.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jun 9, 2022

@jdahlstrom

@dpaoliello wrote in rust-lang/rust#95051 (comment) that #[doc(hidden)] is a fine concession to make if it helps make this happen, and I agree.

text/0000-vec-fallible-allocation.md Outdated Show resolved Hide resolved
text/0000-vec-fallible-allocation.md Outdated Show resolved Hide resolved
dpaoliello and others added 2 commits June 14, 2022 12:31
Co-authored-by: Jonathan Schwender <[email protected]>
Co-authored-by: Jonathan Schwender <[email protected]>
- `FallibleVec` cannot be used where a something explicitly asks for a `Vec` or any of the infallible allocation traits.
- Requires a developer to choose at the type level if they need fallible or infallible allocation (unless there's a way to convert between the types).

### Always return `Result` (with the error based on the allocator), but allow implicit unwrapping for "infallible allocation"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of being generic over the Err type of a Result wouldn't it be possible to make it generic over the return-value itself? This probably needs GATs to have something that produce the return type.

    pub fn try_push(&mut self, value: T) -> A::AllocationResult<()>::Output;

which then produces () or Result<(), AllocError> depending on the allocator.

@scottmcm
Copy link
Member

That would be fine with me. Wrapping &Vec and &mut Vec does not have the downsides that wrapping Vec itself does.

This made me think. Anything that could possibly reallocate an existing Vec necessarily must be &mut self.

That means that one wrapper on &mut Vec<_>, with a convenience method, could be a nice way to split these into their own documentation page and emphasize them in the code. Like vec.checked_allocations().reserve(n), or something. (Probably with a much better name than that, because that's super-long.)

That would also give the possibility of a similar wrapper for use-existing-capacity versions. Because a major problem with try_push is that I might have thought it would never reallocate, but would instead fail if there wasn't available capacity. And then vec.existing_capacity().push(x) could be the way to spell that kind of fallibility. (Again under a better name than that one.)

@jdahlstrom
Copy link

jdahlstrom commented Jul 24, 2022

That means that one wrapper on &mut Vec<_>, with a convenience method, could be a nice way to split these into their own documentation page and emphasize them in the code. Like vec.checked_allocations().reserve(n), or something. (Probably with a much better name than that, because that's super-long.)

Yes, this was pretty much what I had in mind. Or if you need to do multiple fallible operations in sequence, just let foo = vec.bikeshed() and NLL will give you the original Vec back once you're done.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Aug 4, 2022

I hope we can reach a decision on this soon? Rust on Linux with a fork along these lines is inching closer to being accepted, and (even if it is post merge) I would really like to get them off a fork.

The bottom line to me is that whatever we end up stabilizing will be closer to the proposed interim solution here than the status quo. We're reworking the implementation (never mind the interface on top) in ways I think we're extremely unlikely to regret.

@Ericson2314
Copy link
Contributor

@the8472 that barely helps this work. That is, sorry, mostly "pretend helpful". The majority of the logic is disabled by that CFG, and yet with very slight modifications works fallibly. That copy-paste-and-modify-slightly you propose is the tragedy of wasted effort (who will maintain it, and keep it in sync with normal Vec?) that is hardly better than the fork situation we are in today.

We want to avoid duplicated work so there is no doubling the maintainership cost. I encourage you to look at the implementation associated with this RFC. The stop gap may be subpar because try_ is gross and no_global_oom_handling, but it is a complete success in deduplicating the code with no runtime overhead.

@the8472
Copy link
Member

the8472 commented Oct 19, 2022

That is, sorry, mostly "pretend helpful"

No, it does help the standard library maintainers by minimizing the changes needed. So it certainly is helpful there. I acknowledge that it shifts the burden to 3rd-party maintainers. But that's what it means to not include all the batteries. fallible allocation is a niche area and it is imo not unreasonable to ask for experimentation to happen outside the standard library.

"it has to be in alloc or a fork" seems like a false dichotomy to me.

I encourage you to look at the implementation associated with this RFC

I did. For example I see some macros there. Do they need to live in alloc?

with no runtime overhead

But with compiletime overhead. I believe my suggestion would avoid that.

That copy-paste-and-modify-slightly you propose

I'm not proposing to copy-paste, you're bringing that up again. You could also write a simple implementation to get things working, without all the bells and whistles. A fallible library probably doesn't want Extend and FromIterator anyway because they'd throw away at least some items or the iterator on error and that may not be what one wants and a new interface would be needed with slightly different behavior.
That the PR touches Extend at all and introduces a try_extend is suspect.

@ojeda
Copy link

ojeda commented Oct 19, 2022

fallible allocation is a niche area

For a systems programming language? How so?

@the8472
Copy link
Member

the8472 commented Oct 19, 2022

The ratio of strict no-oom-panic rust projects vs. those that don't have such a policy. Or the time it took since Rust 1.0 for this feature to be pushed? But really, this doesn't seem like a productive tangent. Replace "niche" with "novel (in rust ecosystem) capability" if you want. This has happened before with the async ecosystem or the previously discussed hashbrown crate.

The RFC says

but in combination with custom fallible allocation extension methods or a FallibleVec wrapper type provided by an external crate such as fallible_collections. However, once the no_global_oom_handling CFG is enabled, basic Vec methods like push are removed, making it difficult and unsafe to implement fallible equivalents outside of the standard library.

But then doesn't discuss in Alternatives what would be minimally necessary to make the "external crate" approach work. The option seems to be discarded immediately without further discussion.

I think overall this RFC has the issue that it's either dismissing or ignoring alternatives. And when that is discussed in this thread then the argument shifts to it being a stop-gap solution. But if it really is a stop-gap solution (and any suggested alternative would be too much work?) then that's not really covered in the RFC, what the planned path forward is, why we need a stop-gap-solution at all, why it's worth the cost and discussion of stop-gap-alternatives (rather than long-term-alternatives).

@Ericson2314
Copy link
Contributor

Ericson2314 commented Oct 20, 2022

@the8472 your non-fork non-copy paste alternative is simply for us to emprovish ourselves with a reduced feature set. That is simply laundering the inefficiency of maintaining a duplicative second implementation: choosing to duplicate less by having less.

@the8472
Copy link
Member

the8472 commented Oct 20, 2022

No. For example the current try_extend impl relies on the entire Extend specialization machinery even though it does not make sense in fallible contexts. Leaving it out and having a simpler vaguely-extend-like feature would need to be done anyway, to get a proper API for fallible use. It's a separate implementation - which could be written in a single function - either way, whether it lives in alloc or in an external crate.

I.e. that PR inflates the apparent code-sharing and thus overstates the case for having it in alloc.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 20, 2022

fallible allocation is a niche area

For a systems programming language? How so?

std and to some extend alloc were originally designed for user space programs. Most user space programs will never see any allocation failure, because of memory overcommitment. (They'll just get killed when they actually to use the memory, even if the allocation succeeded.) Many non-user space usages of Rust are no_std and don't use std and often also don't use alloc. That's why std/alloc doesn't have good support for falible allocation yet, even though Rust has been around as a systems programming language for quite a few years.

I very much do think we should better support fallible allocation in alloc. No disagreement there. There are many more places than the Linux kernel were fallible allocation is useful.

However, this RFC is not about whether we should support fallible allocation in alloc. It's also not about a specific design we should adopt to provide that. It's about a "stop-gap" solution to "unblock important work", without the intention of stablizing any of the proposed methods.

Which is the minimal set of tools that you are missing to be able to implement a Vec that fits your needs?

@m-ou-se this is really a not-helpful framing. No one wants to recreate the standard API from scratch. Even if it is safe. Vec currently has excellent documentation, familiar names, etc. and we want to reusae all those things! [..]

But you don't want to "reuse all those things", right? That's why we have this no_global_oom_handling hack to rip out the majority of the Vec interface. (Not just methods, but also a lot of the trait implementations like Extend and From<&[T]> and so on.)

We need to just accept that FallibleVec on top of infallible Vec is always going to suck. Until we all accept that premise this conversation is just going to go in circles.

Adding and maintaining fifteen new try_ methods that will never be stabilized is also going to 'suck'.

I very much understand that "just wrap Vec" isn't a great solution, either. So that's why I'm asking what tools are needed for a better stop-gap solution than that.

Duplicating half the interface with a try_ prefix is going to be consfusing (we already have e.g. try_insert on other structures with a different meaning), involves a lot of new code we need to maintain, and only results in something that satisfies most of the needs when using nightly/unstable Rust together with the no_global_oom_handling hack.

What work is currently blocked that this RFC would unblock? What are the other ways to unblock it that require fewer changes to the standard library (especially fewer never-to-be-stabilized items)?

@m-ou-se
Copy link
Member

m-ou-se commented Oct 21, 2022

I had a spare hour, so I experimented a bit. I implemented this "FallibleVec" idea in a separate crate (using stable Rust), including all methods proposed in this RFC, with only the no_global_oom_handling part of Vec. Just to get an idea of how feasible it is and what issues one might run into with the set of tools currently provided by alloc.

There weren't many issues I ran into, except:

  • The fancy specializations we have in alloc aren't usable. Specifically: Extend and FromIterator have nice specializaitons, but can't be used in a Result<_, TryReserveError> way.
    (Similarly, extend_from_slice and extend_from_within have separate Clone and Copy variants through specialization. (That doesn't have to be a big deal. Just requiring T: Copy isn't unreasonable, and avoids the issue.))

  • Shrinking is tricky. It's possible on stable, using alloc::realloc() and from_raw_parts, but it isn't pretty. (Also, realloc() won't give a TryReserveError, and you can't create this error yourself on stable. Creating a new TryShrinkError is a reasonable option though, since TryReserveErrorKind::CapacityOverflow can't happen while shrinking anyway.)

Nearly all other things I had to implement came down first reserving and then afterwards (if that succeeded) unsafely assuming there's enough space to add new items. Most of that was relatively easy, thanks to .spare_capacity_mut(), which already allows for (safe!) writing to the unused capacity. The unsafe part is .set_len() to update the length afterwards. (Perhaps we should have something similar to .spare_capacity_mut() that returns an object that represents the unused capacity that could also safely update the vec's length. Basically putting the .push_within_capacity() functionality into this object.)

@m-ou-se
Copy link
Member

m-ou-se commented Oct 21, 2022

Another thing I realized while trying to use/test some of my code, is that the Clone trait is pretty useless in fallible context. Having a .try_clone() method on Vec<i32> works, but on Vec<Vec<String>> it'd also need to recursively use the fallible clone methods of the nested Vec and Strings. We'd need to have some TryClone trait and derive macro for cloning to be useful in places where we can't just panic on allocation. That issue isn't addressed by this RFC. (Would try_extend_from_slice just assume cloning the elements never fails/panics?)

@ojeda
Copy link

ojeda commented Oct 22, 2022

The ratio of strict no-oom-panic rust projects vs. those that don't have such a policy. Or the time it took since Rust 1.0 for this feature to be pushed? But really, this doesn't seem like a productive tangent. Replace "niche" with "novel (in rust ecosystem) capability" if you want. This has happened before with the async ecosystem or the previously discussed hashbrown crate.

I see what you meant, and yes, those projects may be niche within Rust so far, but anybody evaluating a systems programming language will expect this kind of feature (fallible allocations and others).

New systems programming languages are not picked up overnight into complex projects, so of course it takes time to get users that request that sort of feature.

And as long as those features aren't supported, Rust will have a harder time being picked up by particular projects, which in turn will make it harder to justify the resources to support them within Rust, and so on and so forth.

Personally, I think it is important to clarify whether Rust is supposed to cover these use cases or not.

std and to some extend alloc were originally designed for user space programs. Most user space programs will never see any allocation failure, because of memory overcommitment. (They'll just get killed when they actually to use the memory, even if the allocation succeeded.) Many non-user space usages of Rust are no_std and don't use std and often also don't use alloc. That's why std/alloc doesn't have good support for falible allocation yet, even though Rust has been around as a systems programming language for quite a few years.

That is fine, and explains why we are in the current situation, but I wanted to point out that, if Rust is to be considered a systems programming language, then this sort of feature should not be relegated due to being niche.

In any case, note that overcommit is disabled in some use cases; personally I have worked in that kind of environment, and the processes we ran there would have needed both alloc and std, had they been written in Rust.

I very much do think we should better support fallible allocation in alloc. No disagreement there. There are many more places than the Linux kernel were fallible allocation is useful.

Indeed.

@ssokolow
Copy link

if Rust is to be considered a systems programming language

Technically, by the original 1972 definition of systems programming, which is more concerned with whether the language prioritizes maintainability of large, complex infrastructure projects with long lifecycles and shifting maintainership, Go is a perfectly valid systems programming language in the age of microservices and what you're talking about is more "low-level programming".

They just got conflated in the era when hardware constraints forced a strong dichotomy between low-level systems programming and high-level scripting.

@the8472
Copy link
Member

the8472 commented Oct 22, 2022

Personally, I think it is important to clarify whether Rust is supposed to cover these use cases or not.

Note that I wasn't arguing against inclusion at all. Just that I'm skeptical that this needs to be rushed into the standard library (and it does seem rushed) and if it does then the RFC doesn't make a good argument for that.

I'm in favor of starting with only lower-level building blocks that would help an external, no_std, no_global_oom_handling-compatible crate to work out practical higher level abstractions.

As I wrote in another comment I found the proposed try_from_iter and try_extend APIs questionable and thus not a good demonstration of the supposed alloc-code-reuse. Similarly @m-ou-se comments that try_extend_from_slice and try_from_elem{_in} are problematic in no-oom contexts.

Imo the set of methods should be pared down and each method should get a short explanation what it enables in an external implementation. Ideally it would be written based on implementation experience.

E.g. I suspect that a fallible-alloc ecosystem will need to develop some sort of TryFromIterator trait, maybe rely on the upper-bound of size hints (which is very uncommon in the infallible context) and other high-level features we haven't thought of in alloc.
Those new things will go through several versions as new stakeholders come out of the woodworks and see some of their use-cases not covered.
I agree that alloc should enable that development. But I have not seen a convincing justification why it should be the place for it.

For the linux kernel specifically and its desire(?) to vendor code I think a mid-term goal could be vendoring an unmodified alloc plus a version of a hypothetical fallible_vec_extensions crate, which could also be published to crates.io so that other projects could benefit from it too.
At some point in the future when the APIs have been polished that crate (or parts thereof) could be upstreamed into alloc.

@ojeda
Copy link

ojeda commented Oct 24, 2022

Technically, by the original 1972 definition of systems programming, which is more concerned with whether the language prioritizes maintainability of large, complex infrastructure projects with long lifecycles and shifting maintainership, Go is a perfectly valid systems programming language in the age of microservices and what you're talking about is more "low-level programming".

That article is just arguing for a change in current usage. It even quotes Rob Pike saying that calling Go one generated confusion. It does not even claim a particular definition is the original one.

If you want to push for such a change, that is fine, but this is not the place, nor the way to do it. In fact, meanwhile a substantial amount of people don't agree, it is better to stick to the ones most people will understand.

By the way, note that using the "low-level" term has a different meaning for people who do not even consider C low-level, so you will likely cause further confusion, even if the separation of concepts proposed by the article is worth it.

For the linux kernel specifically and its desire(?) to vendor code I think a mid-term goal could be vendoring an unmodified alloc

If it is unmodified, then we wouldn't need to vendor it (this is one of the motivations of the RFC, see below).

At some point in the future when the APIs have been polished that crate (or parts thereof) could be upstreamed into alloc.

As far as I understand, this is what John and Daniel are trying to do here, except in-tree in an explicitly unstable manner, due to the context behind this: originally (more than a year ago), the kernel plan was to have a forked/re-implemented alloc, which would be expected to diverge substantially. But we had a call with Josh, John et al. who requested us to avoid doing so in order to give a chance to alloc to provide the required interfaces upstream. We agreed to try that approach, so they went ahead and starting providing no_global_oom_handling, this RFC, etc., so that we could drop the dependency or, at least, minimize the divergence. So they kept their promise and I greatly appreciate their work, since it also simplified my life downstream and started the ball rolling on these important use cases.

Now, doing it in an external crate may be OK, but it is not what we discussed originally, and it would be nice to understand the implications.

For instance, who will maintain it? You explicitly mentioned shifting the burden to a third-party, and that is a major change. Instead, I would have expected the external crate to be still considered part of the standard library, as a blessed crate or similar. If not, would PRs in the main repo still require passing tests in the crate? Who will be the arbitrer on what goes in when several projects start using it? How will unstable features (if any) be handled? Etc.

Please note the key benefit for the kernel with the current approach is precisely that alloc is maintained upstream, that the code is being heavily tested, that the tests are required to pass with any Rust change/release, etc. I suspect for others projects that may also be the case (@randomPoison?). If that is lost, then it is more likely that alloc diverges in different projects to adapt it to whatever they need, which is what was agreed to attempt to avoid.

@the8472
Copy link
Member

the8472 commented Oct 24, 2022

But we had a call with Josh, John et al. who requested us to avoid doing so in order to give a chance to alloc to provide the required interfaces upstream. We agreed to try that approach, so they went ahead and starting providing no_global_oom_handling, this RFC, etc., so that we could drop the dependency or, at least, minimize the divergence.

Ok, I wasn't aware of there being such an agreement and I don't know how far that goes.

If that is lost, then it is more likely that alloc diverges in different projects to adapt it to whatever they need, which is what was agreed to attempt to avoid.

I'm still only reading "upstream or fork". This seems like a very... C-ish mindset. Why not write a library? I simply do not see this discussed enough in the RFC, the arguments that are there don't strike me as convincing. Especially in the light of Mara cobbling together an experiment in an hour, on stable. I know, I know, something production-ready would take more time, but still... it would be a start to inform this RFC at least.

Please note the key benefit for the kernel with the current approach is precisely that alloc is maintained upstream, that the code is being heavily tested, that the tests are required to pass with any Rust change/release, etc. I suspect for others projects that may also be the case

If some interested parties came together and wrote a crate and tests and so on you'd get roughly the same? In the end std doesn't run on magic reliability dust. We only run tests for tier-1 platforms, we don't have 100% branch coverage (possibly not even 100% function coverage) and not all unstable features are equally well-tested (some are dog-fooded, others not).
Granted, there are way more eyeballs and also crater runs. But that may not help as much with unstable niche features!

You explicitly mentioned shifting the burden to a third-party, and that is a major change. Instead, I would have expected the external crate to be still considered part of the standard library, as a blessed crate or similar. If not, would PRs in the main repo still require passing tests in the crate? Who will be the arbitrer on what goes in when several projects start using it? How will unstable features (if any) be handled? Etc.

I'm not that great with processes, I was mostly imagining the no-oom-panic stakeholders getting together and building something which they could use in the near future and improve on. Things could go in under feature flags. API breaks would be enabled by major version bumps. After some rounds of improvements those parts could be upstreamed in the mid-future and deprecated in the lib, which would eventually only contain some bells and whistles that wouldn't fit well into alloc.
E.g. if we decide we don't want a try_vec! macro in alloc that could still live in a crate.

Parallel to that work we could also start adding some low-level building blocks to alloc sooner, informed by code actually written in that crate. But that would be a smaller set than this RFC.


Just for clarification. I'm only weakly lobbying for this particular outcome. Consider this as a slightly more verbose "why don't you just [...]" style comment and then tell me why I'm wrong.

Comment on lines +71 to +98
try_vec!

impl<T> Vec<T> {
pub fn try_with_capacity(capacity: usize) -> Result<Self, TryReserveError>;
pub fn try_from_iter<I: IntoIterator<Item = T>>(iter: I) -> Result<Vec<T>, TryReserveError>;
}

impl<T, A: Allocator> Vec<T, A> {
pub fn try_append(&mut self, other: &mut Self) -> Result<(), TryReserveError>;
pub fn try_extend<I: IntoIterator<Item = T>>(&mut self, iter: I, ) -> Result<(), TryReserveError>;
pub fn try_extend_from_slice(&mut self, other: &[T]) -> Result<(), TryReserveError>;
pub fn try_extend_from_within<R>(&mut self, src: R) -> Result<(), TryReserveError> where R: RangeBounds<usize>; // NOTE: still panics if given an invalid range
pub fn try_insert(&mut self, index: usize, element: T) -> Result<(), TryReserveError>; // NOTE: still panics if given an invalid index
pub fn try_into_boxed_slice(self) -> Result<Box<[T], A>, TryReserveError>;
pub fn try_push(&mut self, value: T) -> Result<(), TryReserveError>;
pub fn try_resize(&mut self, new_len: usize, value: T) -> Result<(), TryReserveError>;
pub fn try_resize_with<F>(&mut self, new_len: usize, f: F) -> Result<(), TryReserveError> where F: FnMut() -> T;
pub fn try_shrink_to(&mut self, min_capacity: usize) -> Result<(), TryReserveError>;
pub fn try_shrink_to_fit(&mut self) -> Result<(), TryReserveError>;
pub fn try_split_off(&mut self, at: usize) -> Result<Self, TryReserveError> where A: Clone; // NOTE: still panics if given an invalid index
pub fn try_with_capacity_in(capacity: usize, alloc: A) -> Result<Self, TryReserveError>;
}

#[doc(hidden)]
pub fn try_from_elem<T: Clone>(elem: T, n: usize) -> Result<Vec<T>, TryReserveError>;

#[doc(hidden)]
pub fn try_from_elem_in<T: Clone, A: Allocator>(elem: T, n: usize, alloc: A) -> Result<Vec<T, A>, TryReserveError>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Summary section says that this RFC aims to unblock important work. Is this the minimal set of functions necessary to do that?

In particular concerns have been raised about

  • try_from_iter
  • try_extend
  • try_from_elem and try_from_elem_in

Additionally try_into_boxed_slice seems potentially problematic because it would drop data on error, i.e. it doesn't allow recovery.

And try_with_capacity strikes me as more of a convenience function for new() + try_reserve.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 25, 2022

Please note the key benefit for the kernel with the current approach is precisely that alloc is maintained upstream, that the code is being heavily tested, that the tests are required to pass with any Rust change/release, etc.

All that is only true for the stable parts of std.

the kernel plan was to have a forked/re-implemented alloc, which would be expected to diverge substantially

There are quite a few comments in this thread that basically come down to "it's either this RFC or a diverging fork", which I don't understand.

Pretty much everything this RFC proposes can be built on top of alloc's (unmodified, stable!) interface, today (even when restricting to just the no_global_oom_handling parts). No fork necessary. (Only try_shrink_to was quite annoying, but I'd be happy to review a PR that adds just that function to alloc.)

It seems to me that if a divergent fork was necessary before this RFC, it'd still be necessary afterwards. This RFC doesn't change any of that.

This RFC doesn't address a fallible version of the ToOwned and Clone traits, or a variant of the TryFrom trait that's specific to allocation failures, or a fallible Extend mechanism, etc. etc., which seems to me like the kind of issues you'll quickly run into. (Which are all things we should probably solve in alloc! But that still requires design work.)

So I'll repeat my question: What does this RFC unblock? If the answer is only "removing the need for a divergent fork" then my follow up question is: Why is a divergent fork of alloc currently needed, and how does this RFC change anything about that?

For instance, who will maintain it?

If the point of this rfc is not to prevent a divergent fork, but just to find a maintainer and home for these (experimental) functions (for as long as there's no stable, well designed, interface in alloc), then this doesn't seem like the right way to go.

New library features tend to be experimented with outside the standard library (e.g. as happens in itertools and futures and so on) and only merged into the standard library when it converges to a stable design. Note that crates like itertools are not a fork of the standard library. They just implement their functionality on top of the (unmodified) standard library.

@Ericson2314
Copy link
Contributor

I think we should have a call on this. The conversation still feels like going in circles. Have you, @m-ou-se, seen the implementation PR that goes with this rust-lang/rust#95051? And do you want to share the thing you whipped together in that hour?

With no_global_oom_handling and the try_ methods added so far we were so close to being done with the original plan. Now with this final step everything is being thrown up in the air and it is somewhat discouraging.

Creating new traits like TryClone would be great, but IMO is best done as follow-up work. Any cool things the allocation working group comes up with are also cool but best done as follow-up work.

So to answer the questions:

Why is a divergent fork of alloc currently needed, and how does this RFC change anything about that?

Separately implementing the featutres is possible, but far more cumbersome. Maintaining the fork is quite possibly easier, maintaining the work was also done with the understanding that @ojeda that hopefully alloc would have the functionality (even unstable) next step, not some external crate as an intermediate hoop to jump through.

If the point of this rfc is not to prevent a divergent fork...

So first to reiterate, the idea isn't that it is not that it is impossible to avoid a fork without this, but that is it burdensome to do so. With these changes no fork is not burdensome.

...but just to find a maintainer and home for these (experimental) functions (for as long as there's no stable, well designed, interface in alloc), then this doesn't seem like the right way to go.

The use of "experiment" still feels misleading to me. Decisions like wrapper types vs try_ are not "experimenting" in my book, but simple bikeshedding over namespacing. The disabling of global OOM handling and adding Result feels entirely rote to me; the semantics are extremely clear and thus there is no experimenting going in on in any deep sense. Only the syntactic conversion used to separate the old and new methods is being decided. I at least am happy to led the libs team at some point so long as the basic functionality of being able to do thing and get back Results is still there.

@ojeda raise great points about maintainership burden, and I would suspect that the current implementation PR delivers a much better features/new code ratio than anything out of trait. @dpaoliello and I were able to make many methods polymorphic and them wrap them to have the desired types. This wrapping would work with try_ or wrapper types. On the other hand we simply cannot do such code-deduplication tricks with a separate library, unless alloc also depends on that library.

So to the second part of that question, it the point is less selfishly foisting a burden onto alloc on the libs team, then recognizing this is not a zero-sum gain and the burden on both parties togehter is substantially less if we do so.

Bottom lines I think is no one wants to experiment, and no one wants to reimplenent code when these polymorphic implementations that serve both needs well. Precisely because it appears our current implementation is greatly deduplicated --- it seems the burden of having this stuff in-tree is significantly less than burden the rest of us would have from maintaining something out of tree.

@the8472
Copy link
Member

the8472 commented Oct 25, 2022

Bottom lines I think is no one wants to experiment

Want may not be all that relevant, I think it is necessary because some of the proposed API surface is imo not fit the purpose (several have been highlighted, there hasn't been a reply regarding those points yet). You can call it "design work" instead of "experimentation" if you prefer. But usually that's best done with an actual consumer rather than in an ivory tower.

I do have ideas what could be done, but I'm not a fallible-alloc user, so they're probably misguided.

@dpaoliello and I were able to make many methods polymorphic and them wrap them to have the desired types.

As I've said previously, if we discount the questionable APIs then the code-sharing becomes less evident, which weakens that argument.

@the8472
Copy link
Member

the8472 commented Oct 25, 2022

not some external crate as an intermediate hoop to jump through

Crates generally should not seen as a "hoop to jump through" in the rust ecosystem but a way for multiple projects to share common code.

The RFC lists multiple potential stakeholders

For example, in OS Kernel environments (such as Linux), some embedded systems, high-reliability systems (such as databases) and multi-user services (where a single request may fail without the entire service halting, such as a web server).

If each stakeholder makes their own fork that likely is more work than designing a shared fallible-allocation crate.

@ojeda
Copy link

ojeda commented Oct 25, 2022

I'm still only reading "upstream or fork". This seems like a very... C-ish mindset. Why not write a library?

I didn't say that, I said it was more likely, especially if it is a normal crate/library.

As for the C mention, there are countless C libraries out there used as-is, so I am not sure what you mean.

Especially in the light of Mara cobbling together an experiment in an hour, on stable. I know, I know, something production-ready would take more time, but still... it would be a start to inform this RFC at least.

The time to implement any particular approach is not the issue at hand (without taking anything away from John, Daniel, Mara and others that have implemented different approaches over time).

If some interested parties came together and wrote a crate and tests and so on you'd get roughly the same?

Not really, for the reasons you are already agreeing to in the same paragraph, others I mentioned, etc.

But even assuming it were the same, we would still need to get to the point you suggest, i.e. to get all those interested parties to agree on a particular setup, process, etc.

After some rounds of improvements those parts could be upstreamed in the mid-future and deprecated in the lib, which would eventually only contain some bells and whistles that wouldn't fit well into alloc.

That is fine and completely reasonable, but if it is a normal crate, we need to consider the downsides I was asking about.

Parallel to that work we could also start adding some low-level building blocks to alloc sooner, informed by code actually written in that crate.

Yeah, that was basically the agreement from 1.5 years ago (which we appreciated a lot!). The details (whether to do it in-tree or not etc.) are what is different, but I am glad everybody agrees on supporting these use cases.

Just for clarification. I'm only weakly lobbying for this particular outcome. Consider this as a slightly more verbose "why don't you just [...]" style comment and then tell me why I'm wrong.

I appreciate it -- on my side, I am just trying to give the kernel perspective here. I didn't write the RFC, even though I will support that or any other approach that tries to get fallible allocations properly supported in Rust as a use case. Again, originally, I thought we would need a fork! :)

@ojeda
Copy link

ojeda commented Oct 25, 2022

All that is only true for the stable parts of std.

What do you mean? The unstable parts are still maintained -- I am not talking about stability guarantees. Moreover, they would be maintained by rust-lang, rather than a third-party, which is the main point.

Furthermore, tests for unstable bits are required to pass in bors, at least for some targets, right? There is a simple build-test one for no_global_oom_handling, for instance. So I guess you are referring to something else.

There are quite a few comments in this thread that basically come down to "it's either this RFC or a diverging fork", which I don't understand.

Assuming that includes my comment, I didn't mean that. In my reply to @the8472 I was talking about the proposal of developing the code as an independent crate that the kernel and other projects would maintain together somehow.

It seems to me that if a divergent fork was necessary before this RFC, it'd still be necessary afterwards.

Yeah, this is precisely why I clarified a few weeks ago here that, from the kernel side, we don't know yet. Nevertheless, we will do our best to reuse whatever solution you have, and we appreciate Rust thinking about how to support this properly.

And since there seem to be other projects also looking to use something like this, I hope it will be useful even if we don't use it as-is, and they may not need the fork.

New library features tend to be experimented (...)

I think we are all aware of it -- it is not the usual procedure, but that is what resulted from the discussions long ago (i.e. with the Rust side and at least one Rust team member involved).

It may be a good idea to have the call again like John said... :)

@the8472
Copy link
Member

the8472 commented Oct 25, 2022

I think we should have a call on this.

libs-api meetings are most Tuesdays on 15:00 UTC. Schedule/participation discussion can be done ahead of meeting time in the t-libs/meetings zulip channel

As for the C mention, there are countless C libraries out there used as-is, so I am not sure what you mean.

Maybe I'm misunderstanding, but it sounds like you'd consider it lower effort to maintain a fork of alloc than developing a reusable fallible-allocation crate? Imo with good package management it's usually the other way around. So adding an extension-crate should be lower-effort than maintaining a fork.

@randomPoison
Copy link

To follow up a bit on how this RFC intersects with the Trusty team: Based on some of the alternatives brought up in this thread, we've determined that we can replace our internal fallible Vec extension trait with the functionality provided by fallible_collections and use Clippy's disallowed methods lint to ban infallible allocation APIs. This isn't quite as robust a solution as using no_global_oom_handling (since infallible APIs still exist, and we may forget to lint for all of them), but it has the advantage for us that it still allows us to use ecosystem crates that may do infallible allocation internally. Our requirements around fallible allocation are less strict than what the Linux kernel folks need, and for the time being we expect to have situations where it may be better for us to use a crate that doesn't support fallible allocation than to build our own solution that does.


That said, I'm still leaning towards wanting this RFC (or one similar to it) to be accepted. A potential advantage of having unstable fallible Vec support in the standard library is how it will enable broader ecosystem support and experimentation. When working with a crate that use Vec internally, I suspect that crate maintainers will generally be more amenable to adding feature-gated support for fallible allocation if doing so only depends on using nightly std features rather than requiring a dependency on another crate; Many crates already have a nightly feature to allow for experimentation using unstable language and standard library features.

I'm in agreement that we can (and should!) experiment with designs for a fallible Vec API in external crates, but I find that crate authors are less willing to depend on such experimental crates until there's some clear community consensus around a specific crate (e.g. serde is a de facto community standard that many crates support behind a feature gate, and the futures crate was a community standard that was used as the basis for a lot of experimental crates). Many crate authors are cautious about what dependencies and feature flags they add, and will avoid adding a dependency without a sufficient level of community consensus around it, especially if that dependency becomes part of their crate's public API.

For Trusty, we're not really worried about having fallible allocation support for our own code: We already have a solution for that, and adding an unstable API to the standard library or finding a community crate to use mainly serves to reduce our internal maintenance burden. The bigger issue is ensuring that ecosystem crates we want to use support fallible allocation. I suspect that we will have an easier time working with crate authors to address this with an unstable API in std than with a "stable" crate (though I don't have hard evidence to back up that claim, and I suspect others will feel differently).


If this RFC is going to be discussed in the next libs team meeting, I will try to attend in order to represent the Trusty use case 🙂

@ojeda
Copy link

ojeda commented Oct 26, 2022

Agreed with Nicole there.

Maybe I'm misunderstanding, but it sounds like you'd consider it lower effort to maintain a fork of alloc than developing a reusable fallible-allocation crate? Imo with good package management it's usually the other way around. So adding an extension-crate should be lower-effort than maintaining a fork.

Package management hasn't anything to do with it. The technical work to add the dependency is not important, and you could use the extension crate approach in-tree too.

@m-ou-se m-ou-se removed the I-libs-api-nominated Indicates that an issue has been nominated for prioritizing at the next libs-api team meeting. label Dec 6, 2022
@tgross35
Copy link
Contributor

I haven't really seen it mentioned here, but could this perhaps interact nicely with the Storages proposal? https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Recruiting.3A.20Storage.20API.20Project.20Group I believe @CAD97was heading this up.

I believe the goal there is to replace the Allocator API with something that could be generic over where the data is stored: heap, array on the stack, MMIO, some buffer... This API will have the benefit that it has to be "invisible" in the same way that Allocator is. But it seems like maybe you could have both a fallible storage and an infallible storage that use the global allocator, and do abstraction over that.

// Pretend these are storages with our default allocator
struct InfallibleSt;
struct FallibleSt;

struct Vec<T, Storage = InfallibleSt>

impl<T> Vec<T, FallibleSt> {
    fn new() -> Self { ... }
    // for when you temporarily want the other kind
    fn as_infallible(&mut self) -> &mut Vec<T, InfallibleSt> { ... }
    fn into_infallible(self) -> Vec<T, InfallibleSt> { ... }
}

impl<T> Vec<T, InfallibleSt> {
    fn new_infallible() -> Self { ... }
}

impl<T, S: Storage> Vec<T, S> {
    // AllocResult<T> = T for `InfallibleSt`, `Result<T, AllocError>` for fallible
    fn push(&mut self, val: T) -> S::AllocResult<T> { ... }
    ...
}

I think something like that is a necessity for the storages API anyway, since a vector on static memory would of course have to be fallible. Unfortunately, the proposal also seems quite far away. So I don't think it's worth blocking on in favor of nightly-only try_xxx functions if that helps, with the intent to never stabilize (in favor of storages or something better)

@dpaoliello
Copy link
Contributor Author

Closing this for now.
In the short term, I've open sourced the FallibleVec trait we've been using at Microsoft: https://crates.io/crates/fallible_vec
In the long term, hopefully Keyword Generics will provide a solution for this: https://blog.rust-lang.org/inside-rust/2022/07/27/keyword-generics.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.