Skip to content

Add #[track_caller] to panicking Vec functions #83909

Closed
Dylan-DPC-zz wants to merge 4 commits intorust-lang:masterfrom
Dylan-DPC-zz:min/add-track-caller
Closed

Add #[track_caller] to panicking Vec functions #83909
Dylan-DPC-zz wants to merge 4 commits intorust-lang:masterfrom
Dylan-DPC-zz:min/add-track-caller

Conversation

@Dylan-DPC-zz
Copy link

@Dylan-DPC-zz Dylan-DPC-zz commented Apr 6, 2021

Rework of #79323
(opened #83359 on rust-lang remote instead of my fork, so decided to create a new one instead)

r? @joshtriplett

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2021
@Dylan-DPC-zz
Copy link
Author

Dylan-DPC-zz commented Apr 6, 2021

merging as per comment

@bors r=joshtriplett

@bors
Copy link
Collaborator

bors commented Apr 6, 2021

📌 Commit d54e4b3 has been approved by joshtriplett

@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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2021
@the8472
Copy link
Member

the8472 commented Apr 6, 2021

There have been some additional optimizations to some of those methods in the last few weeks and they are highly sensitive to perturbations, so another perf run might make sense.

@Dylan-DPC-zz
Copy link
Author

Dylan-DPC-zz commented Apr 6, 2021

fair enough

@bors r-
@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

rust-timer commented Apr 6, 2021

Awaiting bors try build completion.

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

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 6, 2021
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 6, 2021
@bors
Copy link
Collaborator

bors commented Apr 6, 2021

⌛ Trying commit d54e4b3 with merge 6f4b2295cd3fc357d384b497bf347321211fbfa9...

@bors
Copy link
Collaborator

bors commented Apr 6, 2021

☀️ Try build successful - checks-actions
Build commit: 6f4b2295cd3fc357d384b497bf347321211fbfa9 (6f4b2295cd3fc357d384b497bf347321211fbfa9)

@rust-timer
Copy link
Collaborator

rust-timer commented Apr 6, 2021

Queued 6f4b2295cd3fc357d384b497bf347321211fbfa9 with parent d322385, future comparison URL.

@rust-timer
Copy link
Collaborator

rust-timer commented Apr 6, 2021

Finished benchmarking try commit (6f4b2295cd3fc357d384b497bf347321211fbfa9): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 6, 2021
@the8472
Copy link
Member

the8472 commented Apr 6, 2021

Looks like there actually are fewer regressions compared to the run in #83359

@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Apr 6, 2021
Closed
@Dylan-DPC-zz
Copy link
Author

Dylan-DPC-zz commented Apr 6, 2021

@bors r=joshtriplett

@bors
Copy link
Collaborator

bors commented Apr 6, 2021

📌 Commit d54e4b3 has been approved by joshtriplett

@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. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2021
@Dylan-DPC-zz
Copy link
Author

Dylan-DPC-zz commented Apr 6, 2021

@bors r-

oops forgot a few places

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 6, 2021
@the8472
Copy link
Member

the8472 commented Apr 6, 2021

But those were in the previous PR? That could explain why there were fewer regressions.

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2021
@estebank
Copy link
Contributor

estebank commented Apr 23, 2021

Should this PR add a test to catch any potential regressions in the panic behavior?

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 9, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 31, 2021
@JohnCSimon
Copy link
Member

JohnCSimon commented May 31, 2021

merge conflicts

@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 15, 2021
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 4, 2021
@Dylan-DPC-zz
Copy link
Author

Dylan-DPC-zz commented Jul 4, 2021

closing this for the moment. will rework it later when I have the time for it

@GrigorenkoPV GrigorenkoPV mentioned this pull request Jun 16, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2024
@bors
Auto merge of rust-lang#126557 - GrigorenkoPV:vec_track_caller, r=<try>
579ad25 
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 added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2024
@bors
Auto merge of rust-lang#126557 - GrigorenkoPV:vec_track_caller, r=<try>
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 added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2024
@bors
Auto merge of rust-lang#126557 - GrigorenkoPV:vec_track_caller, r=joboet
f6648f2 
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)
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Oct 14, 2024
@bors
Auto merge of #126557 - GrigorenkoPV:vec_track_caller, r=joboet
70a31c0 
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/rust#79323 (comment))"

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[^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 #83909 was opened instead.

By the time #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/rust#83359 (comment)
[^thinking]: rust-lang/rust#83359 (comment)
[^ok]: rust-lang/rust#83359 (comment)
[^typo]: rust-lang/rust#83359 (comment)
[^remote]: rust-lang/rust#83359 (comment)
[^optimizations]: rust-lang/rust#83909 (comment)
[^perf2]: rust-lang/rust#83909 (comment)
[^ok2]: rust-lang/rust#83909 (comment)
[^tests]: rust-lang/rust#83909 (comment)
[^conflicts]: rust-lang/rust#83909 (comment)
[^rip]: rust-lang/rust#83909 (comment)
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Oct 17, 2024
@bors
Auto merge of #126557 - GrigorenkoPV:vec_track_caller, r=joboet
1b9e2cb 
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/rust#79323 (comment))"

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[^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 #83909 was opened instead.

By the time #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/rust#83359 (comment)
[^thinking]: rust-lang/rust#83359 (comment)
[^ok]: rust-lang/rust#83359 (comment)
[^typo]: rust-lang/rust#83359 (comment)
[^remote]: rust-lang/rust#83359 (comment)
[^optimizations]: rust-lang/rust#83909 (comment)
[^perf2]: rust-lang/rust#83909 (comment)
[^ok2]: rust-lang/rust#83909 (comment)
[^tests]: rust-lang/rust#83909 (comment)
[^conflicts]: rust-lang/rust#83909 (comment)
[^rip]: rust-lang/rust#83909 (comment)
Kobzol pushed a commit to Kobzol/portable-simd that referenced this pull request Feb 3, 2026
@bors
Auto merge of #126557 - GrigorenkoPV:vec_track_caller, r=joboet
3af09dc 
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/rust#79323 (comment))"

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[^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 #83909 was opened instead.

By the time #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/rust#83359 (comment)
[^thinking]: rust-lang/rust#83359 (comment)
[^ok]: rust-lang/rust#83359 (comment)
[^typo]: rust-lang/rust#83359 (comment)
[^remote]: rust-lang/rust#83359 (comment)
[^optimizations]: rust-lang/rust#83909 (comment)
[^perf2]: rust-lang/rust#83909 (comment)
[^ok2]: rust-lang/rust#83909 (comment)
[^tests]: rust-lang/rust#83909 (comment)
[^conflicts]: rust-lang/rust#83909 (comment)
[^rip]: rust-lang/rust#83909 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@joshtriplett joshtriplett

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.