Revert "Use drop_guard in some places in {core,alloc,std}" - #162128
Conversation
This reverts commit 4f404c1.
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? clarfonthey (Since we don't need to review clean reverts.) |
|
@bors p=1 rollup=iffy note="Perf regression revert" |
No need to apologize, this is totally normal 😉 This is why we run perf on every merge. Perf effects can be pretty non-obvious and unpredictable, I would have trouble predicting this one too. |
|
I mean, in hindsight, it obviously would have affected perf, but you know :p |
Looks like bors didn't pick it up. Is there |
|
I was doing it without r+ because the tests hadn't passed yet, and wanted to wait for PR CI first just in case it wasn't actually a clean merge. And now I can just do this: @bors r+ |
|
@bors rollup=iffy note="Reverting regression" |
|
@bors rollup=never |
|
That is extremely fair; I know that we currently unroll for perf so I wasn't sure what the status quo is. |
|
We unroll for perf to create builds that can be benchmarked, but triaging the performance of rollups by running benchmarks is still manual. I'm currently working on automating it |
This comment has been minimized.
This comment has been minimized.
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 5db7f4b (parent) -> edc52f8 (this PR) Test differencesShow 1528 test diffs1528 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard edc52f87c28f328c61685a02c47887a5cec7d767 --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 (edc52f8): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@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 -0.9%, secondary -1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -1.0%, secondary 4.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -1.3%, secondary -1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.082s -> 472.928s (-0.45%) |
cc @panstromek @GrigorenkoPV
Reverts this PR due to perf issues, so that a fix can be verified to not affect perf. Apologies for not initially running perf; kinda tunnel-visioned "this just does the same thing" without considering the changes from non-generic functions to generic closures.