-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Remove most #[track_caller] from allocating Vec methods
#147042
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.
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
Conversation
|
Let's get a metric, @bors2 try @rust-timer queue I think it would be worth a comment somewhere in the vec module about the downsides here because it seems likely to eventually come up again. |
This comment has been minimized.
This comment has been minimized.
Remove most `#[track_caller]` from allocating Vec methods
This comment has been minimized.
This comment has been minimized.
efe3c53 to
4241b54
Compare
|
I've also reverted the part of #142728 that were related to allocation (and kept those related to normal panics). |
They cause significant binary size overhead while contributing little value. Also removes them from the wrapping String methods that do not panic.
4241b54 to
9316d45
Compare
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (53ff5a2): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 3.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.5%, secondary 0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.4%, secondary -0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 471.619s -> 469.77s (-0.39%) |
|
Well that’s about the easiest set of wins we’ve had in a while 🎉. One note here is that the extend impls effectively take a callback, so I think this would give a less accurate panic location if the Iterator panics? Doesn’t seem concerning to me, that should basically never happen. I still think it wouldn’t hurt to add a comment somewhere in vec/mod.rs to help ward off potential future contributions adding this back without knowing the history here. r=me after that |
|
I'm not sure anyone would find such a comment any more than they would find this PR. rustc-perf would certainly present a barrier, and I can surely hope that the next time there is more attention to the feature actually doing what people want 😨 |
|
Fair enough, |
|
Scheduling: Bumping some rollup=never PRs so they get a fair chance to compete with rollups. @bors p=5 |
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 848e674 (parent) -> c7f6aa2 (this PR) Test differencesShow 834 test diffs834 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard c7f6aa2869acdbf014d094c6e427e554e160b6db --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (c7f6aa2): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.8%, secondary -3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.7%, secondary 3.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.4%, secondary -0.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 471.059s -> 470.617s (-0.09%) |
…ss35 Remove most `#[track_caller]` from allocating Vec methods They cause significant binary size overhead while contributing little value. closes rust-lang#146963, see that issue for more details.
They cause significant binary size overhead while contributing little value.
closes #146963, see that issue for more details.