Add #[track_caller] to panicking Vec functions #83359

Dylan-DPC-zz · 2021-03-22T01:00:25Z

Rework of #79323

cc @nvzqz

r? @joshtriplett

Dylan-DPC-zz · 2021-03-22T15:27:06Z

@bors try

bors · 2021-03-22T15:27:16Z

⌛ Trying commit af60851 with merge 29b6a20637e5eac3c6da17e198fdbb0905e70db5...

bors · 2021-03-22T16:15:11Z

☀️ Try build successful - checks-actions
Build commit: 29b6a20637e5eac3c6da17e198fdbb0905e70db5 (29b6a20637e5eac3c6da17e198fdbb0905e70db5)

nagisa · 2021-03-22T19:11:42Z

This definitely needs a performance evaluation, in both microbenchmark and real use-case scenarios. #[track_caller] may be cheap, but its probably not as free as one would like.

Dylan-DPC-zz · 2021-03-22T19:26:03Z

@bors try @rust-timer queue

rust-timer · 2021-03-22T19:26:04Z

Awaiting bors try build completion.

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

bors · 2021-03-22T19:26:11Z

⌛ Trying commit af60851 with merge 4419c96369613ee87683c99b73992ab49319a5f7...

bors · 2021-03-22T20:26:58Z

☀️ Try build successful - checks-actions
Build commit: 4419c96369613ee87683c99b73992ab49319a5f7 (4419c96369613ee87683c99b73992ab49319a5f7)

rust-timer · 2021-03-22T20:27:00Z

Queued 4419c96369613ee87683c99b73992ab49319a5f7 with parent 2287a88, future comparison URL.

rust-timer · 2021-03-22T22:58:30Z

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

joshtriplett · 2021-03-23T21:45:06Z

There's a minor performance hit by timing metrics, but the hit on memory (max-rss) seems more excessive.

Thomasdezeeuw · 2021-03-31T13:24:12Z

library/alloc/src/vec/mod.rs

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


This seems unintentional.

joshtriplett · 2021-04-02T22:22:31Z

Following up on this: I've since learned that the max-rss statistics aren't necessarily reliable enough to know if the observed increase in memory here is real or spurious. The performance numbers seem reasonable given the benefit, so I think it's reasonable for this change to go ahead (modulo the typo fix already noted).

Dylan-DPC-zz · 2021-04-02T23:13:43Z

@joshtriplett once i fix the typo, can i r= you?

joshtriplett · 2021-04-03T00:07:14Z

@Dylan-DPC Yes, go ahead.

Dylan-DPC-zz · 2021-04-06T12:06:48Z

created #83909 because i did this on the wrong remote :D

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)

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)

Dylan-DPC added 3 commits March 22, 2021 01:47

add track_caller to vector

9c116c7

fix "imaginary cat on keyboard" moment

94a89da

fix "imaginary cat on keyboard" moment

f263de4

rust-highfive assigned joshtriplett Mar 22, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2021

added some to mod.rs

e486a9a

This comment has been minimized.

Sign in to view

fix track caller call

af60851

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 22, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 22, 2021

Dylan-DPC-zz mentioned this pull request Mar 23, 2021

add #[track_caller] to slice split_at_mut fn #83403

Closed

Thomasdezeeuw reviewed Mar 31, 2021

View reviewed changes

Dylan-DPC-zz mentioned this pull request Apr 6, 2021

Add #[track_caller] to panicking Vec functions #83909

Closed

Dylan-DPC-zz closed this Apr 6, 2021

Dylan-DPC-zz deleted the min/add-track-caller branch April 6, 2021 12:06

the8472 mentioned this pull request Sep 19, 2022

Add track_caller to iterator methods #102024

Open

GrigorenkoPV mentioned this pull request Jun 16, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add #[track_caller] to panicking Vec functions #83359

Add #[track_caller] to panicking Vec functions #83359

Dylan-DPC-zz commented Mar 22, 2021

This comment has been minimized.

Dylan-DPC-zz commented Mar 22, 2021

bors commented Mar 22, 2021

bors commented Mar 22, 2021

nagisa commented Mar 22, 2021 •

edited

Loading

Dylan-DPC-zz commented Mar 22, 2021

rust-timer commented Mar 22, 2021

bors commented Mar 22, 2021

bors commented Mar 22, 2021

rust-timer commented Mar 22, 2021

rust-timer commented Mar 22, 2021

joshtriplett commented Mar 23, 2021

Thomasdezeeuw Mar 31, 2021

joshtriplett commented Apr 2, 2021

Dylan-DPC-zz commented Apr 2, 2021

joshtriplett commented Apr 3, 2021

Dylan-DPC-zz commented Apr 6, 2021

Add #[track_caller] to panicking Vec functions #83359

Add #[track_caller] to panicking Vec functions #83359

Conversation

Dylan-DPC-zz commented Mar 22, 2021

This comment has been minimized.

Dylan-DPC-zz commented Mar 22, 2021

bors commented Mar 22, 2021

bors commented Mar 22, 2021

nagisa commented Mar 22, 2021 • edited Loading

Dylan-DPC-zz commented Mar 22, 2021

rust-timer commented Mar 22, 2021

bors commented Mar 22, 2021

bors commented Mar 22, 2021

rust-timer commented Mar 22, 2021

rust-timer commented Mar 22, 2021

joshtriplett commented Mar 23, 2021

Thomasdezeeuw Mar 31, 2021

Choose a reason for hiding this comment

joshtriplett commented Apr 2, 2021

Dylan-DPC-zz commented Apr 2, 2021

joshtriplett commented Apr 3, 2021

Dylan-DPC-zz commented Apr 6, 2021

nagisa commented Mar 22, 2021 •

edited

Loading