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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Fix perf regression from #113569 #113630

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d63e43
perf: don't deinit protected fn args
RalfJung Jul 13, 2023
0ee0889
interpret: avoid some clone() in argument handling
RalfJung Jul 13, 2023
9834a41
TEMP: try to reproduce earlier perf results
RalfJung Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions compiler/rustc_const_eval/src/interpret/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,13 +421,17 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
/// Called on places used for in-place function argument and return value handling.
///
/// These places need to be protected to make sure the program cannot tell whether the
/// argument/return value was actually copied or passed in-place..
/// argument/return value was actually copied or passed in-place.
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 {
Copy link
Member

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(..)] ?

Copy link
Member Author

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.

ecx.write_uninit(place)?;
}
Ok(())
}

/// Called immediately before a new stack frame gets pushed.
Expand Down
14 changes: 7 additions & 7 deletions compiler/rustc_const_eval/src/interpret/terminator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,13 @@ impl<'tcx, Prov: Provenance> FnArg<'tcx, Prov> {
impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// Make a copy of the given fn_arg. Any `InPlace` are degenerated to copies, no protection of the
/// original memory occurs.
pub fn copy_fn_arg(
pub fn copy_fn_arg<'a>(
&self,
arg: &FnArg<'tcx, M::Provenance>,
) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> {
arg: &'a FnArg<'tcx, M::Provenance>,
) -> InterpResult<'tcx, Cow<'a, OpTy<'tcx, M::Provenance>>> {
match arg {
FnArg::Copy(op) => Ok(op.clone()),
FnArg::InPlace(place) => self.place_to_op(&place),
FnArg::Copy(op) => Ok(Cow::Borrowed(op)),
FnArg::InPlace(place) => Ok(Cow::Owned(self.place_to_op(&place)?)),
}
}

Expand All @@ -56,7 +56,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
&self,
args: &[FnArg<'tcx, M::Provenance>],
) -> InterpResult<'tcx, Vec<OpTy<'tcx, M::Provenance>>> {
args.iter().map(|fn_arg| self.copy_fn_arg(fn_arg)).collect()
args.iter().map(|fn_arg| self.copy_fn_arg(fn_arg).map(|x| x.into_owned())).collect()
}

pub fn fn_arg_field(
Expand Down Expand Up @@ -637,7 +637,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// An `InPlace` does nothing here, we keep the original receiver intact. We can't
// really pass the argument in-place anyway, and we are constructing a new
// `Immediate` receiver.
let mut receiver = self.copy_fn_arg(&args[0])?;
let mut receiver = self.copy_fn_arg(&args[0])?.into_owned();
let receiver_place = loop {
match receiver.layout.ty.kind() {
ty::Ref(..) | ty::RawPtr(..) => {
Expand Down