Add #[track_caller] to panicking Vec functions #83359
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
|
@bors try |
|
⌛ Trying commit af60851 with merge 29b6a20637e5eac3c6da17e198fdbb0905e70db5... |
|
☀️ Try build successful - checks-actions |
|
This definitely needs a performance evaluation, in both microbenchmark and real use-case scenarios. |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
|
⌛ Trying commit af60851 with merge 4419c96369613ee87683c99b73992ab49319a5f7... |
|
☀️ Try build successful - checks-actions |
|
Queued 4419c96369613ee87683c99b73992ab49319a5f7 with parent 2287a88, future comparison URL. |
|
Finished benchmarking try commit (4419c96369613ee87683c99b73992ab49319a5f7): 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 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 |
|
There's a minor performance hit by timing metrics, but the hit on memory (max-rss) seems more excessive. |
| @@ -2792,7 +2807,7 @@ impl<T, A: Allocator, const N: usize> TryFrom<Vec<T, A>> for [T; N] { | |||
| /// # Examples | |||
| /// | |||
| /// ``` | |||
| /// use std::convert::TryInto; | |||
| /// use std::convert::TryInto;k | |||
There was a problem hiding this comment.
This seems unintentional.
|
Following up on this: I've since learned that the |
|
@joshtriplett once i fix the typo, can i r= you? |
|
@Dylan-DPC Yes, go ahead. |
|
created #83909 because i did this on the wrong remote :D |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 16, 2024
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
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
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
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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rework of #79323
cc @nvzqz
r? @joshtriplett