-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
fallible allocation experiment 2 #117738
fallible allocation experiment 2 #117738
Conversation
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
The job Click to see the possible cause of the failure (guessed by this bot)
|
type Result<T, E> = T; | ||
} | ||
|
||
/// Handle allocation failure falliblyby returning a `Result`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Handle allocation failure falliblyby returning a `Result`. | |
/// Handle allocation failure fallibly by returning a `Result`. |
|
||
/// Handle allocation failure globally by panicking / aborting. | ||
#[derive(Debug)] | ||
pub struct Fatal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub struct Fatal; | |
#[non_exhaustive] | |
pub enum Fatal {} |
Maybe?
You mistyped “type alias” as “trait alias” in the PR description. |
This was a very intentional decision to fix page size explosions (issue #115718, Zulip discussion, PR #116471). |
Oh I know why that was chosen. To be clear, I'm saying that |
I'm considering moving the generic parameter into Allocator: trait Allocator<FH: FailureHanding = Fatal> The benefit of this is that for the normal case, nobody needs to worry about what that third generic parameter does. One question is whether the |
Wasn't this the approach you already experimented with in PR #111970, which this PR supercedes?
I am afraid that I think there are two distinct usecases, which may be at odds. On the one hand, moving the allocation failure strategy to the allocator may possibly provide a performance benefit, both internally and to consumers, by eliminating the need to check for a potential failure at every step of the way and by (possibly) improving return argument passing ( I would argue that this is a case for making the error on an associated type on the It also seems orthogonal to support for fallible allocations in containers, to be honest. On the other hand, a single container or algorithm may have different strategies for handling allocation failure depending on the context. This is seen here by In such case, the allocator is NOT in position to take the decision, which leaves us with two choices:
And if the allocator doesn't handle fallibility, then it's not clear why it should be concerned about So if the only argument is "one less argument on containers", I must admit I find it fairly shallow:
|
No, that had an associated type on Allocator.
I know, it was just an example.
I really don't understand what you mean by this. Can you expand on this? Thinking about this again, I think we could just implement impl<T, A: Allocator<Fatal>> Vec<T, A> {
fn try_reserve(&mut self, additional: usize) -> Result<(), TryReserveError>
where A: Allocator<Fallible>
{ ... }
} Because if someone has an Everything you said below the line makes sense to me. |
I'd be wary of going down that road, as it would be a pain for code generic over the fallibility.
Good point, I read the default argument as being an associated type constraint somehow. I am not sure if this would play well with specialization. On stable, at least, it's no possible to implement
Then again, arguably, an allocator that is unaware of any FailureHandler cannot specialize for it either, so (2) may not be an issue? (It does seem to make the question about |
Well the plan is that only two will ever exist: Fatal and Fallible. So my idea would be that if you only support one or the other, you only implement one or the other. Otherwise you'd implement both |
This makes me wonder whether this would be better modeled with a const generic parameter, specifically a const generic enum, rather than a (sealed, I presume?) trait. Though I am not sure where the support's at... |
I'm not sure about the status of |
Im going to mark this as S-experimental, this doesn't look like it's waiting for a review. |
Closing this as it was an experiment |
supercedes #111970
The idea is to implement fallible allocation APIs using an extra generic parameter on the container type. Here that has been done for
Vec
. The docs turned out much better but there is still some room for improvement.In order to limit the impact on the
std
docs, I used a type alias forstd::vec::Vec
as @matthieu-m suggested. This actually produced quite nice docs complete with all of the relevant impls, and excluding the duplicate functions for the irrelevantFallible
case.There are a few things we'll need to handle on the rustdoc side before moving forward with this, some of which can be seen in the following image. I think we can address all of them by making
#[doc(inline)]
work on trait aliases. It would need to do the following:alloc::vec::Vec
docs)impl
headers (in this case,FH
should be hidden)Unrelated to
doc(inline)
, it would be really nice if there was a way to copy the docs of one module to another. As it is, I think the only way to make sure thatstd::vec
stays up-to-date with thealloc::vec
docs will be to extract the docs to a separate text file and useinclude_str!
.Documentation archive
(Hopefully that still mostly works, I had to delete a couple things)