-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Fix perf regression from #113569 #113630
Fix perf regression from #113569 #113630
Conversation
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 155e495e23a82fc96295035c63b4b9b063d20e03 with merge fd512b4af0b2ecc12d3420a0adf2643f4bac7c1c... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (fd512b4af0b2ecc12d3420a0adf2643f4bac7c1c): comparison URL. Overall result: ✅ improvements - 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 658.224s -> 658.42s (0.03%) |
That seems to be most of the regressions, nice. Now let's see which part it is... |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit b076064403189411fed337a918339d7b7b6608b2 with merge f169694109fb7fbc06109ccd3dc5130a2a5ed7de... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (f169694109fb7fbc06109ccd3dc5130a2a5ed7de): comparison URL. Overall result: ✅ improvements - 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 658.224s -> 657.81s (-0.06%) |
Looks like the write_uninit does cause a noticeable regression. It could also help UB detection, but this kind of UB is somewhat unlikely -- I think you can't even trigger it without custom MIR. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (17fdc9f67e2cd4f61473cc8dc14be7f6eb3e5d9e): comparison URL. Overall result: ✅ improvements - 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.
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: 657.596s -> 658.308s (0.11%) |
fn protect_in_place_function_argument( | ||
ecx: &mut InterpCx<'mir, 'tcx, Self>, | ||
place: &PlaceTy<'tcx, Self::Provenance>, | ||
) -> InterpResult<'tcx> { | ||
// Without an aliasing model, all we can do is put `Uninit` into the place. | ||
ecx.write_uninit(place) | ||
// This can only be violated with custom MIR though so we avoid the perf hit. | ||
if ecx.tcx.sess.opts.unstable_opts.extra_const_ub_checks { |
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.
@RalfJung , this flag has to be turned on explicitly, right?
Can/should we also unconditionally apply this logic when the code being compiled was defined via #[custom_mir(..)]
?
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.
Indeed this is an off-by-default flag, also guarding whether we check for value validity on every typed copy.
I feel like even with custom MIR, causing UB here is so unlikely that it's not worth the trouble. However, for some reason it seems that just checking this flag is already sufficient to mostly negate all perf benefits? Or the perf changes we saw earlier were mostly noise.
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 9834a41 with merge 5d7b1a9394a85c6f682dd159b16da70ce3434799... |
💔 Test failed - checks-actions |
I don't see a clear error message, no idea what is going on
|
@bors try |
⌛ Trying commit 9834a41 with merge 6d1e5d660bdb5534e55c2d3c6a64de9c4b67e92b... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
The job Click to see the possible cause of the failure (guessed by this bot)
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (6d1e5d660bdb5534e55c2d3c6a64de9c4b67e92b): comparison URL. Overall result: no relevant changes - 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 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.
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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 658.585s -> 659.087s (0.08%) |
Even the full version of this PR (identical to what I tested earlier) cannot find any perf benefits any more. The perf triage in #113569 concluded this is likely mostly spurious. Hence closing this PR. |
Fix perf regression from #113569
r? @oli-obk