-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Add a backtrace to Allocation, display it in leak reports #109061
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
Failed to set assignee to
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit f333ee50016e250b849f35947483f3e28989e3e0 with merge 0a70014f8cc287d3eee38518da13127b0eca278e... |
☀️ Try build successful - checks-actions |
1 similar comment
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (0a70014f8cc287d3eee38518da13127b0eca278e): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
Based on the cachegrind diff, I believe the regressions above are legitimate and caused by this change. This new commit may fix them. |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit c994d6d0f210fd2c4b6bca144347f90299e010cc with merge 77af1a06420c9a7b396987192c44bcd1e4b46bda... |
☀️ Try build successful - checks-actions |
1 similar comment
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (77af1a06420c9a7b396987192c44bcd1e4b46bda): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
Based on the cachegrind diffs all the perf results are optimization instability, they don't have anything to with |
3ebe6e7
to
ba7675b
Compare
4ac6f8f
to
3d658ce
Compare
I completely mispredicted/misunderstood the performance implications here. Based on running this program: fn main() {
let _big: Vec<Box<u8>> = vec![Box::new(0u8); 1024 * 32];
} I can measure 4% memory overhead in Miri from collecting full backtraces to all leakable allocations, and I cannot measure any significant runtime overhead. I think what's going on here is pretty simple: The interpreter is so slow and so memory-hungry already that the expense of creating the full There are clever strategies we could use to reduce the amount of memory overhead to 2% in this microbenchmark, but I think those apply here only because our stack is dominated by the Rust runtime frames. In more interesting programs that drive up the memory usage of the interpreter for good reasons, we would probably not prune many frames from the stacktrace. So I think we'd just be complicating this code path for no significant benefit. |
Fair. IMO it'd still make sense to have a flag to disable backtrace capturing, so that this remains easy to benchmark. |
83b9000
to
6770c2c
Compare
I added a flag that does the thing and a test for it. I "blessed" the test with |
6770c2c
to
cd305dd
Compare
I rebased to pull in the fix that makes Miri tests work again, and re-blessed the middle commit. |
@bors r+ |
📌 Commit cd305dd21938dcce7d79a24b17949327ee088ba1 has been approved by It is now in the queue for this repository. |
This comment has been minimized.
This comment has been minimized.
32-bit tests 🤦 |
Co-authored-by: Ralf Jung <post@ralfj.de>
cd305dd
to
fb68292
Compare
@bors r=oli-obk |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
Finished benchmarking commit (23eb90f): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
This addresses rust-lang/miri#2813
Information like this from diagnostics is indispensable for diagnosing problems that are difficult to reproduce such as https://github.com/rust-lang/miri-test-libstd/actions/runs/4395316008/jobs/7697019211#step:4:770 (which has not been reproduced or diagnosed).