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

add try_reserve and try_reserve_exact to Vec #43890

Closed
wants to merge 1 commit into from

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Aug 15, 2017

Full rationale and proposal document coming Soon(TM), but I believe this is the API/impl that the gecko devs should use to solve their issues.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 15, 2017

@Gankra
Copy link
Contributor Author

Gankra commented Aug 15, 2017

TODO:

  • create tracking issue (and set unstable issue number)
  • publish rationale doc
  • benchmarks?

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

@bors: try

@bors
Copy link
Contributor

bors commented Aug 15, 2017

⌛ Trying commit 4d29038 with merge 1465398...

bors added a commit that referenced this pull request Aug 15, 2017
add try_reserve and try_reserve_exact to Vec

Full rationale and proposal document coming Soon(TM), but I believe this is the API/impl that the gecko devs should use to solve their issues.
@arielb1 arielb1 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 15, 2017
@bors
Copy link
Contributor

bors commented Aug 15, 2017

☀️ Test successful - status-travis
State: approved= try=True

@glandium
Copy link
Contributor

It seems like this only enables the oom handler from https://doc.rust-lang.org/alloc/oom/fn.set_oom_handler.html to be called, and it doesn't seem to allow to recover from the allocation failure.

So, say, for example, you have a browser trying to load a resource that would require to allocate a very large buffer of hundreds of megabytes, and there's not enough consecutive address space free to allocate those hundreds of megabytes. The allocator returns a failure. But then what?

@Gankra
Copy link
Contributor Author

Gankra commented Aug 16, 2017

@glandium I don't understand what you're trying to say. What more could we possibly expose? The collection is left unmodified on failure, so you can do whatever you want.

There are a few patterns users of these APIs follow:

  • Fail the current task (in much the same way as if input couldn't be parsed, or a connection failed, or whatever else), and hope the memory problems clear up (especially likely if the requested allocation was catastrophically large, such as from a malicious input)
  • Take a slower and less memory-intensive path (e.g. move to in-place mergesort after failing to perform normal mergesort)
  • Try to relieve memory pressure in the application (purge caches, garbage collect, etc)

@glandium
Copy link
Contributor

So there are two things. The first is that I missed that try_reserve is to be called before each fallible operation on the vec, which is cumbersome, but enough to get started. The second is that this doesn't cover all the other types that enclose a vec internally (like HashMap).

@Gankra
Copy link
Contributor Author

Gankra commented Aug 16, 2017

HashMap doesn't contain a Vec, but yes it should also have this API. As should VecDeque. Both of these types already have reserve methods so generalizing this to them should be simple. BTreeMap is more difficult, as fallible allocation fundamentally needs to be part of the element insertion process.

This PR is the first step towards developing our story on fallible allocation. It establishes the CollectionAllocErr type, and a basic idiom. This basic idiom is a decent 80/20 solution, as it covers almost every case with minimal API impact. Other collections, such as the fork of libcollections the Gecko devs will likely implement for FF57, can implement this idiom without diverging from the standard library.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 16, 2017

(still finishing up the longform rationale)

/// (usually `isize::MAX` bytes).
CapacityOverflow,
/// Error due to the allocator (see the `AllocErr` type's docs).
AllocErr(AllocErr),
Copy link
Member

Choose a reason for hiding this comment

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

I think you can make an AllocErr from a custom string, so is it worth having the distinction between these two errors at the API level? (e.g. do you think that these'll get matched on and dispatch to different behaviors?)

Copy link
Member

Choose a reason for hiding this comment

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

And the second after I read this I see panics on capacity overflow and oom on allocation errors...

@Gankra
Copy link
Contributor Author

Gankra commented Aug 18, 2017

Rationale: rust-lang/rfcs#2116

@aturon
Copy link
Member

aturon commented Aug 22, 2017

I'm going to close this pending RFC approval.

THANK YOU @gankro for pushing so hard on this topic! We'll try to move quickly on the RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants