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

Vec::try_with_capacity #120504

Merged
merged 3 commits into from
Mar 9, 2024
Merged

Vec::try_with_capacity #120504

merged 3 commits into from
Mar 9, 2024

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Jan 30, 2024

Related to #91913

Implements try_with_capacity for Vec, VecDeque, and String. I can follow it up with more collections if desired.

Vec::try_with_capacity() is functionally equivalent to the current stable:

let mut v = Vec::new();
v.try_reserve_exact(n)?

However, try_reserve calls non-inlined finish_grow, which requires old and new Layout, and is designed to reallocate memory. There is benefit to using try_with_capacity, besides syntax convenience, because it generates much smaller code at the call site with a direct call to the allocator. There's codegen test included.

It's also a very desirable functionality for users of no_global_oom_handling (Rust-for-Linux), since it makes a very commonly used function available in that environment (with_capacity is used much more frequently than all (try_)reserve(_exact)).

@rustbot
Copy link
Collaborator

rustbot commented Jan 30, 2024

r? @joshtriplett

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

@rustbot rustbot added O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 30, 2024
@rust-log-analyzer

This comment has been minimized.

@the8472
Copy link
Member

the8472 commented Jan 30, 2024

I has been a few months since something related came up but my understanding of the api team's position is that adding more try methods is going to face resistance until the fallible Vec / being-generic-over-fallibility question has been settled. Unless it's really important to have that method soon because it's hard to do it with existing code.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@kornelski
Copy link
Contributor Author

I think this one method is relatively uncontroversial. The try_ pattern is already established. I don't think there are any alternatives to it in the foreseeable future. I'm proposing just try_with_capacity here, not doubling of the rest of the API surface.

Even if something as ambitious as the keyword generics initiative made progress, it's likely that you would still need some syntax to opt in to fallibility and to help type inference, and it's hard to beat the try_ prefix on that. So even if some fancy let v = Vec::<~try>::with_capacity()? became possible, try_with_capacity may remain as a convenient shortcut (similar to the desire to have .collect_vec(), despite the type system already being generic enough not to need that method).

The alternative designs that are possible today don't work well. FallibleVec as a trait doesn't play well with type inference, and a simple let v = FallibleVec::try_with_capacity(n)? doesn't compile.

Fallibility as a type, like TryVec, is incompatible with the ubiquitous Vec type. It's difficult to adopt, because it either requires converting back and forth with regular Vec on API boundaries, or it becomes "infectious" in APIs, like a lesser version of sync vs async APIs problem. Duplication of methods isn't as painful as duplication of types.

Rust currently has a third approach — no_global_oom_handling feature flag. with_capacity is not available in environments using it, like Rust-for-Linux. try_with_capacity brings this useful functionality to the strictly-OOM-handling environments.

@rust-log-analyzer

This comment has been minimized.

@the8472
Copy link
Member

the8472 commented Jan 30, 2024

I'm proposing just try_with_capacity here, not doubling of the rest of the API surface.

And the next person also only adds one more method 😉

@rust-log-analyzer

This comment has been minimized.

@kornelski
Copy link
Contributor Author

No, let's be serious. I don't think slippery slope applies here. For example, in #117503 I've added hints that make try_extend_from_slice, try_resize, try_append, and try_insert unnecessary.

Addition of try_with_capacity enables making DIY fallible versions of split_off and shrink_to_fit. Even though that takes a few lines of code, it's probably fine, because these are less used functions.

I don't think try_push is needed, because when used naively it can add a lot of reallocation and error checking overhead. And when allocations are amortized with something like try_with_capacity, then regular push can be safe to use. push_in_capacity or some other safe abstraction for filling spare_capacity_mut would work better.

@the8472
Copy link
Member

the8472 commented Jan 30, 2024

I don't think slippery slope applies here.

I... see the picture differently. Several people have made RFCs and several PRs to add those APIs. Very large API surface, not just 1 or 2 methods. It's listed as a RfL wanted feature. There's an entire crew of slope connoisseurs just waiting to slide it down.
And I even agree that we should have a fallible interface! But I also think salami slicing (intentionally or not) doesn't get us a good design.

And this addition doesn't even seem all that valuable.

Addition of try_with_capacity enables making DIY fallible versions of split_off and shrink_to_fit.

You can do that also with new + try_reserve. Yeah, those won't optimize as well. But various other methods won't either without access to e.g. specialization. I understand the status quo sucks. But imo just one more method won't fix that.

@rust-log-analyzer

This comment has been minimized.

@kornelski
Copy link
Contributor Author

kornelski commented Jan 31, 2024

Those big PRs have been rejected or stalled, so we already know there's a limit to this slope.

This method enables several other Vec methods to become fallible at zero cost. Inclusion of this method could even be an argument against adding other try_ methods, because try_with_capacity() followed by another call can make it fallible for free. For example, try_vec![; n] is not necessary, because try_with_capacity()+resize() compiles to the same code.

Based on quick grep of this repo, outside of tests, there are 524 uses of .with_capacity, 278 uses of .reserve(_exact), 145 .shrink_to_fit, and 17 .split_off. Based on how common these methods are, it's easy to dismiss something like try_split_off as too niche. OTOH, with_capacity is used quite a lot, more than reserve. Given that we have a try_ version of reserve, IMHO it makes sense to have a try_ version of even more common with_capacity.

I think try_with_capacity is a good design for Rust. This method is so obvious that it's been proposed multiple times in the same form. Fallible APIs have been discussed a lot, RFCs have been open for years, and no perfect design has been found. Duplicate try_ methods are not elegant, but they're exceptionally simple and pragmatic. They're backwards-compatible with existing containers, and can be adopted gradually. They don't require any new syntax, and are still locally explicit. They don't require addition of more complex generics to Rust. They work fine with type inference as-is.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@kornelski kornelski force-pushed the try_with_capacity branch from 55b85cc to 784e6a1 Compare March 1, 2024 18:24
@Amanieu
Copy link
Member

Amanieu commented Mar 4, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 4, 2024

📌 Commit 784e6a1 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
…nieu

Vec::try_with_capacity

Related to rust-lang#91913

Implements try_with_capacity for `Vec`, `VecDeque`, and `String`. I can follow it up with more collections if desired.

`Vec::try_with_capacity()` is functionally equivalent to the current stable:

```rust
let mut v = Vec::new();
v.try_reserve_exact(n)?
```

However, `try_reserve` calls non-inlined `finish_grow`, which requires old and new `Layout`, and is designed to reallocate memory. There is benefit to using `try_with_capacity`, besides syntax convenience, because it generates much smaller code at the call site with a direct call to the allocator. There's codegen test included.

It's also a very desirable functionality for users of `no_global_oom_handling` (Rust-for-Linux), since it makes a very commonly used function available in that environment (`with_capacity` is used much more frequently than all `(try_)reserve(_exact)`).
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 7, 2024
…nieu

Vec::try_with_capacity

Related to rust-lang#91913

Implements try_with_capacity for `Vec`, `VecDeque`, and `String`. I can follow it up with more collections if desired.

`Vec::try_with_capacity()` is functionally equivalent to the current stable:

```rust
let mut v = Vec::new();
v.try_reserve_exact(n)?
```

However, `try_reserve` calls non-inlined `finish_grow`, which requires old and new `Layout`, and is designed to reallocate memory. There is benefit to using `try_with_capacity`, besides syntax convenience, because it generates much smaller code at the call site with a direct call to the allocator. There's codegen test included.

It's also a very desirable functionality for users of `no_global_oom_handling` (Rust-for-Linux), since it makes a very commonly used function available in that environment (`with_capacity` is used much more frequently than all `(try_)reserve(_exact)`).
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2024
…kingjubilee

Rollup of 13 pull requests

Successful merges:

 - rust-lang#113525 (Dynamically size sigaltstk in std)
 - rust-lang#114009 (compiler: allow transmute of ZST arrays with generics)
 - rust-lang#116793 (Allow targets to override default codegen backend)
 - rust-lang#118623 (Improve std::fs::read_to_string example)
 - rust-lang#120504 (Vec::try_with_capacity)
 - rust-lang#121089 (Remove `feed_local_def_id`)
 - rust-lang#121280 (Implement MaybeUninit::fill{,_with,_from})
 - rust-lang#122087 (Add missing background color for top-level rust documentation page and increase contrast by setting text color to black)
 - rust-lang#122104 (Rust is a proper name: rust → Rust)
 - rust-lang#122110 (Make `x t miri` respect `MIRI_TEMP`)
 - rust-lang#122114 (Make not finding core a fatal error)
 - rust-lang#122115 (Cancel parsing ever made during recovery)
 - rust-lang#122126 (Fix `tidy --bless` on  ̶X̶e̶n̶i̶x̶ Windows)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 9, 2024
…nieu

Vec::try_with_capacity

Related to rust-lang#91913

Implements try_with_capacity for `Vec`, `VecDeque`, and `String`. I can follow it up with more collections if desired.

`Vec::try_with_capacity()` is functionally equivalent to the current stable:

```rust
let mut v = Vec::new();
v.try_reserve_exact(n)?
```

However, `try_reserve` calls non-inlined `finish_grow`, which requires old and new `Layout`, and is designed to reallocate memory. There is benefit to using `try_with_capacity`, besides syntax convenience, because it generates much smaller code at the call site with a direct call to the allocator. There's codegen test included.

It's also a very desirable functionality for users of `no_global_oom_handling` (Rust-for-Linux), since it makes a very commonly used function available in that environment (`with_capacity` is used much more frequently than all `(try_)reserve(_exact)`).
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 9, 2024
Rollup of 12 pull requests

Successful merges:

 - rust-lang#99153 (Add Read Impl for &Stdin)
 - rust-lang#112136 (Add std::ffi::c_str module)
 - rust-lang#120504 (Vec::try_with_capacity)
 - rust-lang#121280 (Implement MaybeUninit::fill{,_with,_from})
 - rust-lang#121813 (Misc improvements to non local defs lint implementation)
 - rust-lang#121833 (Suggest correct path in include_bytes!)
 - rust-lang#121860 (Add a tidy check that checks whether the fluent slugs only appear once)
 - rust-lang#122160 (Eagerly translate `HelpUseLatestEdition` in parser diagnostics)
 - rust-lang#122178 (ci: add a runner for vanilla LLVM 18)
 - rust-lang#122186 (Remove a workaround for a bug)
 - rust-lang#122215 (Some tweaks to the parallel query cycle handler)
 - rust-lang#122223 (Fix typo in `VisitorResult`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 9, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#99153 (Add Read Impl for &Stdin)
 - rust-lang#114655 (Make `impl<Fd: AsFd>` impl take `?Sized`)
 - rust-lang#120504 (Vec::try_with_capacity)
 - rust-lang#121280 (Implement MaybeUninit::fill{,_with,_from})
 - rust-lang#121403 (impl From<TryReserveError> for io::Error)
 - rust-lang#121526 (on the fly type casting for `build.rustc` and `build.cargo`)
 - rust-lang#121584 (bump itertools to 0.12)
 - rust-lang#121711 (Implement junction_point)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e3c0158 into rust-lang:master Mar 9, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 9, 2024
Rollup merge of rust-lang#120504 - kornelski:try_with_capacity, r=Amanieu

Vec::try_with_capacity

Related to rust-lang#91913

Implements try_with_capacity for `Vec`, `VecDeque`, and `String`. I can follow it up with more collections if desired.

`Vec::try_with_capacity()` is functionally equivalent to the current stable:

```rust
let mut v = Vec::new();
v.try_reserve_exact(n)?
```

However, `try_reserve` calls non-inlined `finish_grow`, which requires old and new `Layout`, and is designed to reallocate memory. There is benefit to using `try_with_capacity`, besides syntax convenience, because it generates much smaller code at the call site with a direct call to the allocator. There's codegen test included.

It's also a very desirable functionality for users of `no_global_oom_handling` (Rust-for-Linux), since it makes a very commonly used function available in that environment (`with_capacity` is used much more frequently than all `(try_)reserve(_exact)`).
@Nadrieril
Copy link
Member

This had a small perf impact

@Nadrieril Nadrieril added the perf-regression Performance regression. label Mar 10, 2024
@Amanieu
Copy link
Member

Amanieu commented Mar 12, 2024

@kornelski Could you look into the perf regressions? This is most likely due to Vec::with_capacity now requiring more monomorphization/inlining, which increases compile times.

@kornelski kornelski deleted the try_with_capacity branch March 12, 2024 17:28
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2024
Less generic code for Vec allocations

Follow up to rust-lang#120504 (comment) which hopefully makes compilation faster.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
Less generic code for Vec allocations

Follow up to rust-lang#120504 (comment) which hopefully makes compilation faster.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2024
Less generic code for Vec allocations

Follow up to rust-lang#120504 (comment) which hopefully makes compilation faster.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2024
Less generic code for Vec allocations

Follow up to rust-lang#120504 (comment) which hopefully makes compilation faster.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.