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

Replace MemoryExtra by Memory in intptrcast methods #62003

Merged
merged 3 commits into from
Jun 21, 2019
Merged

Replace MemoryExtra by Memory in intptrcast methods #62003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cc28d4
Replace MemoryExtra by Memory in intptrcast methods
pvdrz Jun 20, 2019
752a1a4
Updated tag methods for consistency
pvdrz Jun 20, 2019
e152c38
Fix merge issues
pvdrz Jun 20, 2019
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
6 changes: 3 additions & 3 deletions src/librustc_mir/const_eval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::interpret::{self,
PlaceTy, MPlaceTy, OpTy, ImmTy, Immediate, Scalar,
RawConst, ConstValue,
InterpResult, InterpErrorInfo, InterpError, GlobalId, InterpretCx, StackPopCleanup,
Allocation, AllocId, MemoryKind,
Allocation, AllocId, MemoryKind, Memory,
snapshot, RefTracking, intern_const_alloc_recursive,
};

Expand Down Expand Up @@ -410,7 +410,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
_id: AllocId,
alloc: Cow<'b, Allocation>,
_kind: Option<MemoryKind<!>>,
_memory_extra: &(),
_memory: &Memory<'mir, 'tcx, Self>,
) -> (Cow<'b, Allocation<Self::PointerTag>>, Self::PointerTag) {
// We do not use a tag so we can just cheaply forward the allocation
(alloc, ())
Expand All @@ -419,7 +419,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
#[inline(always)]
fn tag_static_base_pointer(
_id: AllocId,
_memory_extra: &(),
_memory: &Memory<'mir, 'tcx, Self>,
) -> Self::PointerTag {
()
}
Expand Down
17 changes: 8 additions & 9 deletions src/librustc_mir/interpret/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ use rustc::ty::{self, query::TyCtxtAt};

use super::{
Allocation, AllocId, InterpResult, Scalar, AllocationExtra,
InterpretCx, PlaceTy, OpTy, ImmTy, MemoryKind, Pointer,
InterpErrorInfo, InterpError
InterpretCx, PlaceTy, OpTy, ImmTy, MemoryKind, Pointer, Memory
};

/// Whether this kind of memory is allowed to leak
Expand Down Expand Up @@ -178,7 +177,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
id: AllocId,
alloc: Cow<'b, Allocation>,
kind: Option<MemoryKind<Self::MemoryKinds>>,
memory_extra: &Self::MemoryExtra,
memory: &Memory<'mir, 'tcx, Self>,
) -> (Cow<'b, Allocation<Self::PointerTag, Self::AllocExtra>>, Self::PointerTag);

/// Return the "base" tag for the given static allocation: the one that is used for direct
Expand All @@ -188,7 +187,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
/// for cyclic statics!
fn tag_static_base_pointer(
id: AllocId,
memory_extra: &Self::MemoryExtra,
memory: &Memory<'mir, 'tcx, Self>,
) -> Self::PointerTag;

/// Executes a retagging operation
Expand All @@ -212,19 +211,19 @@ pub trait Machine<'mir, 'tcx>: Sized {

fn int_to_ptr(
int: u64,
_extra: &Self::MemoryExtra,
_mem: &Memory<'mir, 'tcx, Self>,
) -> InterpResult<'tcx, Pointer<Self::PointerTag>> {
if int == 0 {
Err(InterpErrorInfo::from(InterpError::InvalidNullPointerUsage))
err!(InvalidNullPointerUsage)
} else {
Err(InterpErrorInfo::from(InterpError::ReadBytesAsPointer))
err!(ReadBytesAsPointer)
}
}

fn ptr_to_int(
_ptr: Pointer<Self::PointerTag>,
_extra: &Self::MemoryExtra,
_mem: &Memory<'mir, 'tcx, Self>,
) -> InterpResult<'tcx, u64> {
Err(InterpErrorInfo::from(InterpError::ReadPointerAsBytes))
err!(ReadPointerAsBytes)
}
}
18 changes: 9 additions & 9 deletions src/librustc_mir/interpret/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {

#[inline]
pub fn tag_static_base_pointer(&self, ptr: Pointer) -> Pointer<M::PointerTag> {
ptr.with_tag(M::tag_static_base_pointer(ptr.alloc_id, &self.extra))
ptr.with_tag(M::tag_static_base_pointer(ptr.alloc_id, &self))
}

pub fn create_fn_alloc(&mut self, instance: Instance<'tcx>) -> Pointer<M::PointerTag> {
Expand Down Expand Up @@ -140,7 +140,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
kind: MemoryKind<M::MemoryKinds>,
) -> Pointer<M::PointerTag> {
let id = self.tcx.alloc_map.lock().reserve();
let (alloc, tag) = M::tag_allocation(id, Cow::Owned(alloc), Some(kind), &self.extra);
let (alloc, tag) = M::tag_allocation(id, Cow::Owned(alloc), Some(kind), &self);
self.alloc_map.insert(id, (kind, alloc.into_owned()));
Pointer::from(id).with_tag(tag)
}
Expand Down Expand Up @@ -327,7 +327,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
fn get_static_alloc(
id: AllocId,
tcx: TyCtxtAt<'tcx>,
memory_extra: &M::MemoryExtra,
memory: &Memory<'mir, 'tcx, M>,
) -> InterpResult<'tcx, Cow<'tcx, Allocation<M::PointerTag, M::AllocExtra>>> {
let alloc = tcx.alloc_map.lock().get(id);
let alloc = match alloc {
Expand Down Expand Up @@ -374,7 +374,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
id, // always use the ID we got as input, not the "hidden" one.
alloc,
M::STATIC_KIND.map(MemoryKind::Machine),
memory_extra
memory
).0)
}

Expand All @@ -387,7 +387,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
// `get_static_alloc` that we can actually use directly without inserting anything anywhere.
// So the error type is `InterpResult<'tcx, &Allocation<M::PointerTag>>`.
let a = self.alloc_map.get_or(id, || {
let alloc = Self::get_static_alloc(id, self.tcx, &self.extra).map_err(Err)?;
let alloc = Self::get_static_alloc(id, self.tcx, &self).map_err(Err)?;
match alloc {
Cow::Borrowed(alloc) => {
// We got a ref, cheaply return that as an "error" so that the
Expand Down Expand Up @@ -416,11 +416,11 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
id: AllocId,
) -> InterpResult<'tcx, &mut Allocation<M::PointerTag, M::AllocExtra>> {
let tcx = self.tcx;
let memory_extra = &self.extra;
let alloc = Self::get_static_alloc(id, tcx, &self);
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I had entirely missed this! This is not what we want. This is the reason for the perf regression. We don't want to ask for a static alloc if we don't absolutely need it. You turned the vast majority of the get_mut calls from one to two HashMap lookups.

Copy link
Member

Choose a reason for hiding this comment

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

This could also explain why MIRI_BACKTRACE=1 made everything so awfully slow when I used it for debugging on CI.

Copy link
Contributor Author

@pvdrz pvdrz Jul 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I remember that change, that was because there was no way to pass self into the get_mut_or without having a mutable and an immutable reference at the same time.

Before doing the MemoryExtra -> Memory change, we passed just self.extra and get_mut_or was called from self.alloc_map so there was no borrowing issues.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's why I am reverting to just MemoryExtra now.

Next time you have to reorder code like that in a way that moves an operation out of a closure and makes it executed much more often than previously... please call that out when submitting the PR, so we can review that specifically. :) I should have seen this in the review, but it gets easier when you call out the "most interesting" bits.

Copy link
Contributor Author

@pvdrz pvdrz Jul 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah sorry, that was a little careless from my part. Next time will do

Edit: I assume we could modify how get_mut_or works or adding a get_mut but I don't think it is worth it

let a = self.alloc_map.get_mut_or(id, || {
// Need to make a copy, even if `get_static_alloc` is able
// to give us a cheap reference.
let alloc = Self::get_static_alloc(id, tcx, memory_extra)?;
let alloc = alloc?;
if alloc.mutability == Mutability::Immutable {
return err!(ModifiedConstantMemory);
}
Expand Down Expand Up @@ -843,7 +843,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
) -> InterpResult<'tcx, Pointer<M::PointerTag>> {
match scalar {
Scalar::Ptr(ptr) => Ok(ptr),
_ => M::int_to_ptr(scalar.to_usize(self)?, &self.extra)
_ => M::int_to_ptr(scalar.to_usize(self)?, self)
}
}

Expand All @@ -854,7 +854,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
) -> InterpResult<'tcx, u128> {
match scalar.to_bits_or_ptr(size, self) {
Ok(bits) => Ok(bits),
Err(ptr) => Ok(M::ptr_to_int(ptr, &self.extra)? as u128)
Err(ptr) => Ok(M::ptr_to_int(ptr, self)? as u128)
}
}
}