-
Notifications
You must be signed in to change notification settings - Fork 13k
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
miri: add some chance to reuse addresses of previously freed allocations #122240
Conversation
The Miri subtree was changed cc @rust-lang/miri Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
There was a problem hiding this comment.
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
This comment has been minimized.
This comment has been minimized.
6dea495
to
1381835
Compare
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 |
63dcaea
to
7e93f8a
Compare
7e93f8a
to
c016768
Compare
@bors r+ |
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 |
☀️ Test successful - checks-actions |
Finished benchmarking commit (9ce37dc): 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)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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 675.107s -> 675.369s (0.04%) |
Now that it landed in Miri and we can do proper benchmarks... without reuse:
With reuse:
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. |
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: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