add Vec::push_within_capacity - fallible, does not allocate

[the8472]

This method can serve several purposes. It

• is fallible
• guarantees that items in Vec aren't moved
• allows loops that do reserve and push separately to avoid pulling in the allocation machinery a second time in the push part which should make things easier on the optimizer
• eases the path towards ArrayVec a bit since - compared to push() - there are fewer questions around how it should be implemented

I haven't named it try_push because that should probably occupy a middle ground that will still try to reserve and only return an error in the unlikely OOM case.

resolves #84649

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[the8472]

Since push and extend_desugared are very hot code:

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 3ad6a9cebd6932b0f8c9b44462442f0f1f08ec46 with merge 1ad6c9ca95479a2d0a3f62df1c16af2064866c0d...

[the8472]

@bors try

[bors]

⌛ Trying commit ce0b668c792325b8e8482ed5bbcad5f305af83fb with merge 00118a4eca2c471243a0886aa38756a6236daa23...

[lukaslueg]

Quick thought: The machinery provided and the example given could be expressed more concise as

let mut vec = Vec::new();
for value in iter {
    // push() will not reallocate
    vec.try_reserve(1).and_then(|v| v.push(value))?;
}
Ok(vec)

if try_reserve(&mut self) -> Result<&mut Self, TryReserveError> instead of Result<(), TryReserveError>.

Am I missing something?

[the8472]

Am I missing something?

push is #[cfg(not(no_global_oom_handling))] which means it's not available in code that doesn't want OOM panics. And it contains a call to reserve() which makes things more difficult for the optimizer when you call it in a loop.

[bors]

☀️ Try build successful - checks-actions
Build commit: 00118a4eca2c471243a0886aa38756a6236daa23 (00118a4eca2c471243a0886aa38756a6236daa23)

[bors]

☀️ Try build successful - checks-actions
Build commit: 00118a4eca2c471243a0886aa38756a6236daa23 (00118a4eca2c471243a0886aa38756a6236daa23)

[rust-timer]

Queued 00118a4eca2c471243a0886aa38756a6236daa23 with parent 60e70cc, future comparison URL.

[Amanieu]

I would prefer if we wrapped the value in a proper error type instead of just returning a raw T. It's a common pattern in the standard library (see BTreeMap::try_insert which returns an OccupiedError).

[JohnTitor]

This hasn't received a review from @kennytm for 10 months so let's re-assign, maybe r? @Amanieu as you left a comment in the past

[Amanieu]

LGTM!

[Amanieu]

Use .unwrap() to show that this cannot fail instead of ignoring the error.

[the8472]

I wrote it this way to avoid panic codegen which can be one of the benefits of this method.

[Dylan-DPC]

@the8472 any updates on this? thanks

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[the8472]

@rustbot ready

[the8472]

I addressed review comments except the one on the doc comment which I disagree with and haven't received further feedback on it. Since the reviewer gave his LGTM I'll take it as approval.

@bors r=amanieu

[bors]

📌 Commit bb74f97 has been approved by amanieu

It is now in the queue for this repository.

[bors]

⌛ Testing commit bb74f97 with merge 1a7c203...

[bors]

☀️ Test successful - checks-actions
Approved by: amanieu
Pushing 1a7c203 to master...

[rust-timer]

Finished benchmarking commit (1a7c203): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	2.9%	[2.9%, 2.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	0.8%	[0.8%, 0.8%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[2.5%, 2.5%]	1

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²