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

GrigorenkoPV · 2024-06-16T17:43:38Z

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

Noratrieb · 2024-06-16T18:12:52Z

@bors try @rust-timer queue

Add `#[track_caller]` to allocating methods of `Vec` & `VecDeque` 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](rust-lang#79323 (comment))" This was first attempted in rust-lang#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 rust-lang#83359. A benchmark was run[^perf], the results (after a bit of thinking[^thinking]) were deemed ok[^ok], but there was a typo[^typo] and the PR was made from a wrong remote in the first place[^remote], so rust-lang#83909 was opened instead. By the time rust-lang#83909 rolled around, the methods in question had received some optimizations[^optimizations], so another perf run was conducted[^perf2]. The results were ok[^ok2]. There was a suggestion to add regression tests for panic behavior [^tests], but before it could be addressed, the PR fell victim to merge conflicts[^conflicts] and died again[^rip]. 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` [^perf]: rust-lang#83359 (comment) [^thinking]: rust-lang#83359 (comment) [^ok]: rust-lang#83359 (comment) [^typo]: rust-lang#83359 (comment) [^remote]: rust-lang#83359 (comment) [^optimizations]: rust-lang#83909 (comment) [^perf2]: rust-lang#83909 (comment) [^ok2]: rust-lang#83909 (comment) [^tests]: rust-lang#83909 (comment) [^conflicts]: rust-lang#83909 (comment) [^rip]: rust-lang#83909 (comment)

bors · 2024-06-16T18:14:03Z

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

bors · 2024-06-16T19:51:53Z

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

rust-timer · 2024-06-16T21:05:05Z

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 · 2024-06-16T21:45:39Z

r? libs

GrigorenkoPV · 2024-06-16T23:14:42Z

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 · 2024-08-18T18:02:07Z

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 · 2024-08-21T13:07:04Z

Sure!
@bors try @rust-timer queue

bors · 2024-08-21T13:08:16Z

⌛ Trying commit 90d80a9 with merge bf493f5...

Add `#[track_caller]` to allocating methods of `Vec` & `VecDeque` 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](rust-lang#79323 (comment))" This was first attempted in rust-lang#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 rust-lang#83359. A benchmark was run[^perf], the results (after a bit of thinking[^thinking]) were deemed ok[^ok], but there was a typo[^typo] and the PR was made from a wrong remote in the first place[^remote], so rust-lang#83909 was opened instead. By the time rust-lang#83909 rolled around, the methods in question had received some optimizations[^optimizations], so another perf run was conducted[^perf2]. The results were ok[^ok2]. There was a suggestion to add regression tests for panic behavior [^tests], but before it could be addressed, the PR fell victim to merge conflicts[^conflicts] and died again[^rip]. 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` [^perf]: rust-lang#83359 (comment) [^thinking]: rust-lang#83359 (comment) [^ok]: rust-lang#83359 (comment) [^typo]: rust-lang#83359 (comment) [^remote]: rust-lang#83359 (comment) [^optimizations]: rust-lang#83909 (comment) [^perf2]: rust-lang#83909 (comment) [^ok2]: rust-lang#83909 (comment) [^tests]: rust-lang#83909 (comment) [^conflicts]: rust-lang#83909 (comment) [^rip]: rust-lang#83909 (comment)

bors · 2024-08-21T15:02:43Z

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

rust-timer · 2024-08-21T16:17:34Z

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%)

As pointed out in rust-lang#126557 (comment) Co-authored-by: Jonas Böttiger <jonasboettiger@icloud.com>

joboet · 2024-08-28T12:58:18Z

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 · 2024-09-02T04:06:53Z

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

joshtriplett · 2024-09-04T16:14:53Z

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 · 2024-09-04T16:24:02Z

Let me do a rebase and resolve the conflicts then

As pointed out in rust-lang#126557 (comment) Co-authored-by: Jonas Böttiger <jonasboettiger@icloud.com>

bors · 2024-09-19T21:33:47Z

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

joboet · 2024-10-13T11:54:17Z

Ship it 🚀!
@bors r+ rollup=never

bors · 2024-10-13T11:54:20Z

📌 Commit 683ef60 has been approved by joboet

It is now in the queue for this repository.

bors · 2024-10-14T02:33:43Z

⌛ Testing commit 683ef60 with merge f6648f2...

bors · 2024-10-14T05:15:13Z

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

rust-timer · 2024-10-14T06:35:08Z

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 · 2024-10-14T07:57:50Z

Ouch on the binary size increases :/

therealprof · 2024-10-14T08:45:54Z

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 · 2024-10-21T18:44:13Z

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 · 2024-10-21T18:45:46Z

@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)

rustbot assigned joshtriplett Jun 16, 2024

rustbot added 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 Jun 16, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 16, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 16, 2024

rustbot assigned m-ou-se and unassigned joshtriplett Jun 16, 2024

GrigorenkoPV force-pushed the vec_track_caller branch from 5451c6d to 90d80a9 Compare August 18, 2024 18:00

rustbot assigned joboet and unassigned m-ou-se Aug 18, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 21, 2024

This comment has been minimized.

Sign in to view

GrigorenkoPV added a commit to GrigorenkoPV/rust that referenced this pull request Aug 22, 2024

Remove an excessive #[track_caller]

848a645

As pointed out in rust-lang#126557 (comment) Co-authored-by: Jonas Böttiger <jonasboettiger@icloud.com>

rustbot added the I-libs-nominated Nominated for discussion during a libs team meeting. label Aug 28, 2024

GrigorenkoPV force-pushed the vec_track_caller branch from 848a645 to 2363faa Compare September 4, 2024 16:26

GrigorenkoPV added a commit to GrigorenkoPV/rust that referenced this pull request Sep 4, 2024

Remove an excessive #[track_caller]

2363faa

As pointed out in rust-lang#126557 (comment) Co-authored-by: Jonas Böttiger <jonasboettiger@icloud.com>

Amanieu removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Sep 18, 2024

GrigorenkoPV added 2 commits September 20, 2024 01:20

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

0d4259e

Bless a test for rust-lang#70963

683ef60

GrigorenkoPV force-pushed the vec_track_caller branch from 2363faa to 683ef60 Compare September 19, 2024 22:21

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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 14, 2024

bors merged commit f6648f2 into rust-lang:master Oct 14, 2024
7 checks passed

rustbot added this to the 1.84.0 milestone Oct 14, 2024

GrigorenkoPV deleted the vec_track_caller branch October 14, 2024 09:03

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 21, 2024

pnkfelix added the I-libs-nominated Nominated for discussion during a libs team meeting. label Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

GrigorenkoPV commented Jun 16, 2024

This comment has been minimized.

Noratrieb commented Jun 16, 2024

This comment has been minimized.

bors commented Jun 16, 2024

bors commented Jun 16, 2024

This comment has been minimized.

rust-timer commented Jun 16, 2024

joshtriplett commented Jun 16, 2024

GrigorenkoPV commented Jun 16, 2024

GrigorenkoPV commented Aug 18, 2024

joboet commented Aug 21, 2024

This comment has been minimized.

bors commented Aug 21, 2024

bors commented Aug 21, 2024

This comment has been minimized.

rust-timer commented Aug 21, 2024

joboet commented Aug 28, 2024

bors commented Sep 2, 2024

joshtriplett commented Sep 4, 2024 •

edited

Loading

GrigorenkoPV commented Sep 4, 2024

bors commented Sep 19, 2024

joboet commented Oct 13, 2024

bors commented Oct 13, 2024

bors commented Oct 14, 2024

bors commented Oct 14, 2024

rust-timer commented Oct 14, 2024

nnethercote commented Oct 14, 2024

therealprof commented Oct 14, 2024

pnkfelix commented Oct 21, 2024

pnkfelix commented Oct 21, 2024

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

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

Conversation

GrigorenkoPV commented Jun 16, 2024

Footnotes

This comment has been minimized.

Noratrieb commented Jun 16, 2024

This comment has been minimized.

bors commented Jun 16, 2024

bors commented Jun 16, 2024

This comment has been minimized.

rust-timer commented Jun 16, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

joshtriplett commented Jun 16, 2024

GrigorenkoPV commented Jun 16, 2024

GrigorenkoPV commented Aug 18, 2024

joboet commented Aug 21, 2024

This comment has been minimized.

bors commented Aug 21, 2024

bors commented Aug 21, 2024

This comment has been minimized.

rust-timer commented Aug 21, 2024

Overall result: ❌ regressions - ACTION NEEDED

joboet commented Aug 28, 2024

bors commented Sep 2, 2024

joshtriplett commented Sep 4, 2024 • edited Loading

GrigorenkoPV commented Sep 4, 2024

bors commented Sep 19, 2024

joboet commented Oct 13, 2024

bors commented Oct 13, 2024

bors commented Oct 14, 2024

bors commented Oct 14, 2024

rust-timer commented Oct 14, 2024

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

nnethercote commented Oct 14, 2024

therealprof commented Oct 14, 2024

pnkfelix commented Oct 21, 2024

pnkfelix commented Oct 21, 2024

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

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

joshtriplett commented Sep 4, 2024 •

edited

Loading