Skip to content

miri: add some chance to reuse addresses of previously freed allocations #122240

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 9, 2024

The hope is that this can help us find ABA issues.

Unfortunately this needs rustc changes so I can't easily run the regular benchmark suite. I used src/tools/miri/tests/pass/float_nan.rs as a substitute:

Before:
Benchmark 1: ./x.py run miri --stage 0 --args src/tools/miri/tests/pass/float_nan.rs --args --edition=2021
  Time (mean ± σ):      9.570 s ±  0.013 s    [User: 9.279 s, System: 0.290 s]
  Range (min … max):    9.561 s …  9.579 s    2 runs

After:
Benchmark 1: ./x.py run miri --stage 0 --args src/tools/miri/tests/pass/float_nan.rs --args --edition=2021
  Time (mean ± σ):      9.698 s ±  0.046 s    [User: 9.413 s, System: 0.279 s]
  Range (min … max):    9.666 s …  9.731 s    2 runs

That's a ~1.3% slowdown, which seems fine to me. I have seen a lot of noise in this style of benchmarking so I don't quite trust this anyway; we can make further experiments in the Miri repo after this migrated there.

r? @oli-obk

@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 9, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 9, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! This name makes so much more sense, I'm surprised I didn't see this earlier

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 9, 2024

Oh wow, somehow the reuse rate is a lot lower on Windows targets. I guess there's a lot of other stuff of the same size being allocated that makes it less likely that we reuse exactly this Box allocation...

@RalfJung RalfJung force-pushed the miri-addr-reuse branch 3 times, most recently from 63dcaea to 7e93f8a Compare March 9, 2024 18:56
@oli-obk
Copy link
Contributor

oli-obk commented Mar 11, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 11, 2024

📌 Commit c016768 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 Mar 11, 2024
@workingjubilee
Copy link
Member

A PR that increases nondet and sounds like it has slightly different system-dependent behavior has bad vibes for a rollup, even if it mostly affects a tool, so

@bors rollup=iffy

@bors
Copy link
Collaborator

bors commented Mar 13, 2024

⌛ Testing commit c016768 with merge 9ce37dc...

@bors
Copy link
Collaborator

bors commented Mar 13, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 13, 2024
@bors bors merged commit 9ce37dc into rust-lang:master Mar 13, 2024
@rustbot rustbot added this to the 1.78.0 milestone Mar 13, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9ce37dc): 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)

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.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.107s -> 675.369s (0.04%)
Artifact size: 310.84 MiB -> 310.83 MiB (-0.00%)

@RalfJung RalfJung deleted the miri-addr-reuse branch March 14, 2024 06:51
@RalfJung
Copy link
Member Author

Now that it landed in Miri and we can do proper benchmarks... without reuse:

$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/backtraces/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/backtraces/Cargo.toml"
  Time (mean ± σ):      1.465 s ±  0.049 s    [User: 1.411 s, System: 0.051 s]
  Range (min … max):    1.405 s …  1.505 s    5 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/invalidate/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/invalidate/Cargo.toml"
  Time (mean ± σ):      9.146 s ±  0.292 s    [User: 9.105 s, System: 0.037 s]
  Range (min … max):    8.670 s …  9.379 s    5 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/mse/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/mse/Cargo.toml"
  Time (mean ± σ):     510.4 ms ±  10.9 ms    [User: 459.9 ms, System: 46.1 ms]
  Range (min … max):   501.1 ms … 526.5 ms    5 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/serde1/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/serde1/Cargo.toml"
  Time (mean ± σ):      1.307 s ±  0.019 s    [User: 1.262 s, System: 0.041 s]
  Range (min … max):    1.294 s …  1.336 s    5 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/serde2/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/serde2/Cargo.toml"
  Time (mean ± σ):      2.814 s ±  0.010 s    [User: 2.764 s, System: 0.046 s]
  Range (min … max):    2.799 s …  2.828 s    5 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/slice-get-unchecked/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/slice-get-unchecked/Cargo.toml"
  Time (mean ± σ):     487.1 ms ±   9.5 ms    [User: 436.7 ms, System: 47.3 ms]
  Range (min … max):   469.2 ms … 494.5 ms    6 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/unicode/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/unicode/Cargo.toml"
  Time (mean ± σ):      1.196 s ±  0.003 s    [User: 1.148 s, System: 0.043 s]
  Range (min … max):    1.191 s …  1.200 s    5 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/zip-equal/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/zip-equal/Cargo.toml"
  Time (mean ± σ):      1.525 s ±  0.039 s    [User: 1.480 s, System: 0.043 s]
  Range (min … max):    1.501 s …  1.591 s    5 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

With reuse:

$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/backtraces/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/backtraces/Cargo.toml"
  Time (mean ± σ):      1.484 s ±  0.035 s    [User: 1.430 s, System: 0.051 s]
  Range (min … max):    1.439 s …  1.512 s    5 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/invalidate/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/invalidate/Cargo.toml"
  Time (mean ± σ):      9.551 s ±  0.091 s    [User: 9.515 s, System: 0.033 s]
  Range (min … max):    9.390 s …  9.604 s    5 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/mse/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/mse/Cargo.toml"
  Time (mean ± σ):     502.3 ms ±  11.5 ms    [User: 457.1 ms, System: 42.3 ms]
  Range (min … max):   489.1 ms … 512.7 ms    6 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/serde1/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/serde1/Cargo.toml"
  Time (mean ± σ):      1.350 s ±  0.033 s    [User: 1.301 s, System: 0.045 s]
  Range (min … max):    1.330 s …  1.409 s    5 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/serde2/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/serde2/Cargo.toml"
  Time (mean ± σ):      2.751 s ±  0.094 s    [User: 2.698 s, System: 0.042 s]
  Range (min … max):    2.669 s …  2.864 s    5 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/slice-get-unchecked/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/slice-get-unchecked/Cargo.toml"
  Time (mean ± σ):     479.8 ms ±   6.3 ms    [User: 430.8 ms, System: 46.2 ms]
  Range (min … max):   470.1 ms … 486.8 ms    6 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/unicode/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/unicode/Cargo.toml"
  Time (mean ± σ):      1.254 s ±  0.036 s    [User: 1.208 s, System: 0.042 s]
  Range (min … max):    1.214 s …  1.284 s    5 runs
 
$ hyperfine -w 1 -m 5 --shell=none "cargo miri run  --manifest-path \"/home/r/src/rust/miri.2/bench-cargo-miri/zip-equal/Cargo.toml\""
Benchmark 1: cargo miri run  --manifest-path "/home/r/src/rust/miri.2/bench-cargo-miri/zip-equal/Cargo.toml"
  Time (mean ± σ):      1.542 s ±  0.004 s    [User: 1.496 s, System: 0.042 s]
  Range (min … max):    1.538 s …  1.546 s    5 runs

That's a slowdown 1.3%, 4.4%, 3.3%, 4.8%, 1.1% for some benchmarks and no change for the rest. So, on the scale at which we're usually measuring Miri speedups, this is not a big change, though it is a bit more than originally anticipated.

# for free to join this conversation on GitHub. Already have an account? # 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
Development

Successfully merging this pull request may close these issues.

8 participants