Skip to content
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

Merged
merged 3 commits into from
Apr 17, 2023

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Mar 12, 2023

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

@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2023

r? @michaelwoerister

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 12, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2023

Failed to set assignee to ghost: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@saethlin
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

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

bors commented Mar 12, 2023

⌛ Trying commit f333ee50016e250b849f35947483f3e28989e3e0 with merge 0a70014f8cc287d3eee38518da13127b0eca278e...

@bors
Copy link
Contributor

bors commented Mar 13, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 13, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0a70014f8cc287d3eee38518da13127b0eca278e): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking 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
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.8% [0.6%, 1.0%] 7
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

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

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.6% [-1.6%, -1.6%] 1
All ❌✅ (primary) - - 0

Cycles

Results

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

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.1% [4.1%, 4.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 13, 2023
@saethlin
Copy link
Member Author

Based on the cachegrind diff, I believe the regressions above are legitimate and caused by this change. This new commit may fix them.

@saethlin
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

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

bors commented Mar 13, 2023

⌛ Trying commit c994d6d0f210fd2c4b6bca144347f90299e010cc with merge 77af1a06420c9a7b396987192c44bcd1e4b46bda...

@bors
Copy link
Contributor

bors commented Mar 13, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 13, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (77af1a06420c9a7b396987192c44bcd1e4b46bda): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking 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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.6% [0.5%, 0.7%] 3
Regressions ❌
(secondary)
0.9% [0.8%, 0.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.3% [-0.4%, -0.2%] 4
All ❌✅ (primary) 0.6% [0.5%, 0.7%] 3

Max RSS (memory usage)

Results

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

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.0% [1.0%, 1.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

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

mean range count
Regressions ❌
(primary)
3.0% [1.3%, 4.9%] 7
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.0% [1.3%, 4.9%] 7

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 13, 2023
@saethlin
Copy link
Member Author

Based on the cachegrind diffs all the perf results are optimization instability, they don't have anything to with Allocation or CTFE.

@saethlin
Copy link
Member Author

saethlin commented Apr 2, 2023

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 Vec<FrameInfo> is just not significant, and is much faster than good pruning logic.

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.

@RalfJung
Copy link
Member

Fair. IMO it'd still make sense to have a flag to disable backtrace capturing, so that this remains easy to benchmark.

@saethlin
Copy link
Member Author

I added a flag that does the thing and a test for it. I "blessed" the test with x run and some copy+paste, since x test miri currently doesn't work.

@saethlin
Copy link
Member Author

I rebased to pull in the fix that makes Miri tests work again, and re-blessed the middle commit.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 16, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Apr 16, 2023

📌 Commit cd305dd21938dcce7d79a24b17949327ee088ba1 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2023
@rust-log-analyzer

This comment has been minimized.

@saethlin
Copy link
Member Author

32-bit tests 🤦

@RalfJung
Copy link
Member

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Apr 16, 2023

📌 Commit fb68292 has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 17, 2023

⌛ Testing commit fb68292 with merge 23eb90f...

@bors
Copy link
Contributor

bors commented Apr 17, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 23eb90f to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Apr 17, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 23eb90f to master...

@bors bors added merged-by-bors This PR was explicitly merged by bors. labels Apr 17, 2023
@bors bors merged commit 23eb90f into rust-lang:master Apr 17, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 17, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (23eb90f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Cycles

Results

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

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.8% [2.1%, 3.6%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
8 participants