Add #[track_caller] to allocating methods of Vec & VecDeque

[GrigorenkoPV]

Part 4 in a lengthy saga.
r? @joshtriplett because they were the reviewer the last 3 times.
@bors rollup=never "just in case this has perf effects, Vec is hot"

This was first attempted in #79323 by @nvzqz. It got approval from @joshtriplett, but rotted with merge conflicts and got closed.

Then it got picked up by @Dylan-DPC-zz in #83359. A benchmark was run¹, the results (after a bit of thinking²) were deemed ok³, but there was a typo⁴ and the PR was made from a wrong remote in the first place⁵, so #83909 was opened instead.

By the time #83909 rolled around, the methods in question had received some optimizations⁶, so another perf run was conducted⁷. The results were ok⁸. There was a suggestion to add regression tests for panic behavior ⁹, but before it could be addressed, the PR fell victim to merge conflicts¹⁰ and died again¹¹.

3 years have passed, and (from what I can tell) this has not been tried again, so here I am now, reviving this old effort.

Given how much time has passed and the fact that I've also touched VecDeque this time, it probably makes sense to
@bors try @rust-timer

Footnotes

1. https://github.com/rust-lang/rust/pull/83359#issuecomment-804450095 ↩

2. https://github.com/rust-lang/rust/pull/83359#issuecomment-805286704 ↩

3. https://github.com/rust-lang/rust/pull/83359#issuecomment-812739031 ↩

4. https://github.com/rust-lang/rust/pull/83359#issuecomment-812750205 ↩

5. https://github.com/rust-lang/rust/pull/83359#issuecomment-814067119 ↩

6. https://github.com/rust-lang/rust/pull/83909#issuecomment-813736593 ↩

7. https://github.com/rust-lang/rust/pull/83909#issuecomment-813825552 ↩

8. https://github.com/rust-lang/rust/pull/83909#issuecomment-813831341 ↩

9. https://github.com/rust-lang/rust/pull/83909#issuecomment-825788964 ↩

10. https://github.com/rust-lang/rust/pull/83909#issuecomment-851173480 ↩

11. https://github.com/rust-lang/rust/pull/83909#issuecomment-873569771 ↩

[Noratrieb]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 5451c6d with merge 579ad25...

[bors]

☀️ Try build successful - checks-actions
Build commit: 579ad25 (579ad2549a130c10ce546563bce542b27309c45e)

[rust-timer]

Finished benchmarking commit (579ad25): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.3%, 0.4%]	4
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-0.4%	[-1.0%, -0.3%]	7
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
All ❌✅ (primary)	-0.1%	[-1.0%, 0.4%]	11

Max RSS (memory usage)

Results (primary -2.4%, secondary -0.5%)

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.

	mean	range	count
Regressions ❌ (primary)	3.6%	[3.6%, 3.6%]	1
Regressions ❌ (secondary)	4.1%	[4.1%, 4.1%]	1
Improvements ✅ (primary)	-5.4%	[-8.7%, -2.0%]	2
Improvements ✅ (secondary)	-5.1%	[-5.1%, -5.1%]	1
All ❌✅ (primary)	-2.4%	[-8.7%, 3.6%]	3

Cycles

Results (primary -1.0%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.0%	[-1.0%, -1.0%]	1

Binary size

Results (primary 0.5%, secondary 1.2%)

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.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.0%, 1.9%]	122
Regressions ❌ (secondary)	1.2%	[0.1%, 1.9%]	84
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.0%, 1.9%]	122

Bootstrap: 671.558s -> 672.209s (0.10%)
Artifact size: 319.87 MiB -> 322.97 MiB (0.97%)

[joshtriplett]

r? libs

[GrigorenkoPV]

The failing test is for #70963. While blessing it would be trivial,

diff --git c/tests/ui/hygiene/panic-location.run.stderr i/tests/ui/hygiene/panic-location.run.stderr
index 5c1778bc485..b9086ecef81 100644
--- c/tests/ui/hygiene/panic-location.run.stderr
+++ i/tests/ui/hygiene/panic-location.run.stderr
@@ -1,3 +1,3 @@
-thread 'main' panicked at library/alloc/src/raw_vec.rs:LL:CC:
+thread 'main' panicked at $DIR/panic-location.rs:LL:CC:
 capacity overflow
 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I am not sure it would still test the thing it is supposed to test.

A minor nitpick, but the comment

rust/tests/ui/hygiene/panic-location.rs

Lines 7 to 8 in 5451c6d

	// The captured stderr from this test reports a location
	// inside `VecDeque::with_capacity`, instead of `<::core::macros::panic macros>`

should probably be changed as well.

[GrigorenkoPV]

I've rebased on top of master, blessed the test, and change its comment a bit.

Might be worth to do another perf run?

r? libs

[joboet]

Sure!
@bors try @rust-timer queue

[bors]

⌛ Trying commit 90d80a9 with merge bf493f5...

[bors]

☀️ Try build successful - checks-actions
Build commit: bf493f5 (bf493f5a29e6cea67c0f28b892572434355c1119)

[rust-timer]

Finished benchmarking commit (bf493f5): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 0.7%]	16
Regressions ❌ (secondary)	0.6%	[0.3%, 1.4%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 0.7%]	16

Max RSS (memory usage)

Results (primary 1.2%, secondary 2.0%)

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.

	mean	range	count
Regressions ❌ (primary)	4.1%	[2.1%, 7.6%]	3
Regressions ❌ (secondary)	3.2%	[2.1%, 4.3%]	3
Improvements ✅ (primary)	-3.0%	[-3.2%, -2.8%]	2
Improvements ✅ (secondary)	-1.7%	[-1.7%, -1.7%]	1
All ❌✅ (primary)	1.2%	[-3.2%, 7.6%]	5

Cycles

Results (primary 3.1%, secondary 4.9%)

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.

	mean	range	count
Regressions ❌ (primary)	3.1%	[1.5%, 3.9%]	3
Regressions ❌ (secondary)	4.9%	[2.7%, 7.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.1%	[1.5%, 3.9%]	3

Binary size

Results (primary 0.5%, secondary 1.1%)

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.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.0%, 2.0%]	121
Regressions ❌ (secondary)	1.1%	[0.0%, 1.8%]	85
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.0%, 2.0%]	121

Bootstrap: 750.629s -> 753.884s (0.43%)
Artifact size: 338.97 MiB -> 339.54 MiB (0.17%)

[joboet]

I don't really have ideas for improving this, but it makes sense to me as a feature, so I'm nominating this for T-libs discussion to see whether they'd be OK with taking the slight performance hit associated with this.

@rustbot label +I-libs-nominated

[bors]

☔ The latest upstream changes (presumably #129063) made this pull request unmergeable. Please resolve the merge conflicts.

[joshtriplett]

We discussed this in today's @rust-lang/libs meeting. We're going to approve this, on the expectation that it won't have appreciable runtime performance overhead. If this does turn out to have appreciable runtime performance overhead, we want to re-evaluate this and consider reverting.

[GrigorenkoPV]

Let me do a rebase and resolve the conflicts then

[bors]

☔ The latest upstream changes (presumably #130572) made this pull request unmergeable. Please resolve the merge conflicts.

[joboet]

Ship it 🚀!
@bors r+ rollup=never

[bors]

📌 Commit 683ef60 has been approved by joboet

It is now in the queue for this repository.

[bors]

⌛ Testing commit 683ef60 with merge f6648f2...

[bors]

☀️ Test successful - checks-actions
Approved by: joboet
Pushing f6648f2 to master...

[rust-timer]

Finished benchmarking commit (f6648f2): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.0%, 0.5%]	26
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.1%]	4
All ❌✅ (primary)	0.3%	[0.0%, 0.5%]	26

Max RSS (memory usage)

Results (primary -3.1%)

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.

	mean	range	count
Regressions ❌ (primary)	5.2%	[5.2%, 5.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.8%	[-9.4%, -3.1%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.1%	[-9.4%, 5.2%]	4

Cycles

Results (secondary 2.2%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.1%, 2.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.5%, secondary 1.1%)

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.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.0%, 2.0%]	119
Regressions ❌ (secondary)	1.1%	[0.1%, 1.9%]	84
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.0%, 2.0%]	119

Bootstrap: 782.102s -> 782.104s (0.00%)
Artifact size: 332.13 MiB -> 332.57 MiB (0.13%)

[nnethercote]

Ouch on the binary size increases :/

[therealprof]

Ouch on the binary size increases :/

Kind of expected. The applications/libraries suffering the most are going to be the ones with the most instantiations of Vec in different places. There might be a workaround for the worst cases by moving the allocations into a separate function. For me the big question really is: are allocation errors that common, that it is worth knowing exactly where they failed? I think the value of knowing which allocation failed on a particular system (most likely due to a OOM situation) is close to non-existent and most likely it's not going to be a single point where this happens but it will happen to any allocation so it's not even useful information for a reproducer.

If anything, it would be more useful to know which kind of allocation failed, i.e. the size of the allocation and the type that was supposed to go into it than the caller.

[pnkfelix]

Visiting for weekly perf triage.

• based on a prior perf run (which predicted 16 primary regressions of roughly the same magnitude as observed here), the T-libs team had already approved this PR under the assumption that there wouldn't be a runtime impact from this.
• there was a note from nnethercote that it isn't totally clear if the binary size increases were anticipated.
• marking as triaged

@rustbot label: +perf-regression-triaged

[pnkfelix]

@rustbot label +I-libs-nominated

just nominating to double check if T-libs count the binary size increases as potential evidence of "runtime regression" as referenced in their earlier comment: #126557 (comment)