Skip to content

Conversation

0xPoe
Copy link
Member

@0xPoe 0xPoe commented Mar 23, 2021

close #83378

@rust-highfive
Copy link
Contributor

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2021
@0xPoe
Copy link
Member Author

0xPoe commented Mar 23, 2021

It looks like many of other methods may also have this issue, do we need to create a new Issue to track and resolve these issues?

@joshtriplett
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 23, 2021
@bors
Copy link
Collaborator

bors commented Mar 23, 2021

⌛ Trying commit f770774 with merge 7a3a18a727383d099db87281fa8973c5b0b40da3...

@bors
Copy link
Collaborator

bors commented Mar 23, 2021

☀️ Try build successful - checks-actions
Build commit: 7a3a18a727383d099db87281fa8973c5b0b40da3 (7a3a18a727383d099db87281fa8973c5b0b40da3)

@rust-timer
Copy link
Collaborator

Queued 7a3a18a727383d099db87281fa8973c5b0b40da3 with parent 2bd94f4, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (7a3a18a727383d099db87281fa8973c5b0b40da3): 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 removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 23, 2021
@Dylan-DPC-zz
Copy link

@hi-rustin once #83359 is merged i'm planning on targeting the others as well

@nagisa
Copy link
Member

nagisa commented Mar 23, 2021

I don't think rust-timer is a good way to evaluate the performance impact of this. This should be ideally accompanied by some microbenchmark results, I feel.

@joshtriplett
Copy link
Member

@nagisa It's not perfect, but it helps give an idea of the impact. A similar proposal to add #[track_caller] to various Vec methods caused a serious performance hit; by contrast, this seems roughly performance-neutral.

@joshtriplett
Copy link
Member

Hmmm. This seems to have a surprisingly high impact on max-rss in some cases.

@Mark-Simulacrum
Copy link
Member

max-rss regularly varies up to 15-20% on benchmarks, though we've not pinpointed why, so I think the variation here is just noise.

@joshtriplett
Copy link
Member

Ah, that's unfortunate; I've seen many people evaluating max-rss figures from rust-timer without knowing about that level of variability.

@crlf0710 crlf0710 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2021
@0xPoe
Copy link
Member Author

0xPoe commented Apr 24, 2021

@joshtriplett Can we merge this PR?

#83359 (comment) It seems that this max-rss change is acceptable?

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2021
@0xPoe
Copy link
Member Author

0xPoe commented Jun 16, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@bors
Copy link
Collaborator

bors commented Jun 16, 2021

@hi-rustin: 🔑 Insufficient privileges: not in try users

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2021
@estebank
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 13, 2021
@bors
Copy link
Collaborator

bors commented Jul 13, 2021

⌛ Trying commit f770774 with merge b1c1d69922eb21f55a08867e6f68fe78e17d2f2b...

@bors
Copy link
Collaborator

bors commented Jul 14, 2021

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

@rust-timer
Copy link
Collaborator

Queued b1c1d69922eb21f55a08867e6f68fe78e17d2f2b with parent 3e1c75c, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (b1c1d69922eb21f55a08867e6f68fe78e17d2f2b): comparison url.

Summary: This benchmark run did not return any significant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 14, 2021
@estebank
Copy link
Contributor

estebank commented Jul 18, 2021

Screen Shot 2021-07-17 at 5 35 12 PM

@0xPoe 0xPoe closed this Jul 20, 2021
@0xPoe 0xPoe deleted the rustin-patch-slice branch July 20, 2021 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

split_at_mut should #[track_caller]