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.

Sign up for GitHub

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

Jump to bottom

Add #[track_caller] to panicking Vec functions #83909

Closed
wants to merge 4 commits into from
Closed

Add #[track_caller] to panicking Vec functions #83909

wants to merge 4 commits into from

Conversation

Dylan-DPC-zz
Copy link

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

merging as per comment

@bors r=joshtriplett

@bors
Copy link
Contributor

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

fair enough

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

@rust-timer
Copy link
Collaborator

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
Contributor

bors commented Apr 6, 2021

⌛ Trying commit d54e4b3 with merge 6f4b2295cd3fc357d384b497bf347321211fbfa9...

@bors
Copy link
Contributor

bors commented Apr 6, 2021

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

@rust-timer
Copy link
Collaborator

Queued 6f4b2295cd3fc357d384b497bf347321211fbfa9 with parent d322385, future comparison URL.

@rust-timer
Copy link
Collaborator

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

@bors r=joshtriplett

@bors
Copy link
Contributor

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

@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

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

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

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