From 97a0b2e2d0816bcf935b7d925f8f1d4225f7def7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Tue, 31 May 2022 12:36:48 +0200 Subject: [PATCH 1/6] ctfe interning: don't walk allocations that don't need it The interning of const allocations visits the mplace looking for references to intern. Walking big aggregates like big static arrays can be costly, so we only do it if the allocation we're interning contains references or interior mutability. Walking ZSTs was avoided before, and this optimization is now applied to cases where there are no references/relocations either. --- .../rustc_const_eval/src/interpret/intern.rs | 23 ++++++++++++++++--- .../rustc_const_eval/src/interpret/memory.rs | 5 ++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs index 1fda60c021eed..808e7db5ae3c0 100644 --- a/compiler/rustc_const_eval/src/interpret/intern.rs +++ b/compiler/rustc_const_eval/src/interpret/intern.rs @@ -168,10 +168,22 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory mplace: &MPlaceTy<'tcx>, fields: impl Iterator>, ) -> InterpResult<'tcx> { - // ZSTs cannot contain pointers, so we can skip them. - if mplace.layout.is_zst() { + // We want to walk the aggregate to look for reference types to intern. While doing that we + // also need to take special care of interior mutability. + // + // As an optimization, however, if the allocation does not contain any pointers: we don't + // need to do the walk. It can be costly for big arrays for example (e.g. issue #93215). + + let Some((size, align)) = self.ecx.size_and_align_of_mplace(&mplace)? else { + // We could be dealing with an extern type here in the future, so we do the regular + // walk. + return self.walk_aggregate(mplace, fields); + }; + + let Some(alloc) = self.ecx.get_ptr_alloc(mplace.ptr, size, align)? else { + // ZSTs cannot contain pointers, so we can skip them. return Ok(()); - } + }; if let Some(def) = mplace.layout.ty.ty_adt_def() { if Some(def.did()) == self.ecx.tcx.lang_items().unsafe_cell_type() { @@ -186,6 +198,11 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory } } + if !alloc.has_relocations() { + // There are no refs or relocations in this allocation, we can skip the interning walk. + return Ok(()); + } + self.walk_aggregate(mplace, fields) } diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index d5e68dbd5b7a9..fb82406ce8246 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -942,6 +942,11 @@ impl<'tcx, 'a, Tag: Provenance, Extra> AllocRef<'a, 'tcx, Tag, Extra> { .check_bytes(&self.tcx, self.range.subrange(range), allow_uninit, allow_ptr) .map_err(|e| e.to_interp_error(self.alloc_id))?) } + + /// Returns whether the allocation has relocations for the entire range of the `AllocRef`. + pub(crate) fn has_relocations(&self) -> bool { + !self.alloc.get_relocations(&self.tcx, self.range).is_empty() + } } impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { From 266bab2ab0659d2c29666e0161238cb4c5abd763 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Mon, 13 Jun 2022 10:30:59 +0200 Subject: [PATCH 2/6] make `get_relocations` private This limits access to the relocations data a bit (instead of increasing it just for the purposes of interning). --- .../rustc_const_eval/src/interpret/memory.rs | 2 +- .../src/mir/interpret/allocation.rs | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index fb82406ce8246..c2a5b71b8f9d4 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -945,7 +945,7 @@ impl<'tcx, 'a, Tag: Provenance, Extra> AllocRef<'a, 'tcx, Tag, Extra> { /// Returns whether the allocation has relocations for the entire range of the `AllocRef`. pub(crate) fn has_relocations(&self) -> bool { - !self.alloc.get_relocations(&self.tcx, self.range).is_empty() + self.alloc.has_relocations(&self.tcx, self.range) } } diff --git a/compiler/rustc_middle/src/mir/interpret/allocation.rs b/compiler/rustc_middle/src/mir/interpret/allocation.rs index 10c4ea63a68b2..1e2b53040d2d7 100644 --- a/compiler/rustc_middle/src/mir/interpret/allocation.rs +++ b/compiler/rustc_middle/src/mir/interpret/allocation.rs @@ -537,21 +537,26 @@ impl Allocation { /// Relocations. impl Allocation { /// Returns all relocations overlapping with the given pointer-offset pair. - pub fn get_relocations(&self, cx: &impl HasDataLayout, range: AllocRange) -> &[(Size, Tag)] { + fn get_relocations(&self, cx: &impl HasDataLayout, range: AllocRange) -> &[(Size, Tag)] { // We have to go back `pointer_size - 1` bytes, as that one would still overlap with // the beginning of this range. let start = range.start.bytes().saturating_sub(cx.data_layout().pointer_size.bytes() - 1); self.relocations.range(Size::from_bytes(start)..range.end()) } + /// Returns whether this allocation has relocations overlapping with the given range. + /// + /// Note: this function exists to allow `get_relocations` to be private, in order to somewhat + /// limit access to relocations outside of the `Allocation` abstraction. + /// + pub fn has_relocations(&self, cx: &impl HasDataLayout, range: AllocRange) -> bool { + !self.get_relocations(cx, range).is_empty() + } + /// Checks that there are no relocations overlapping with the given range. #[inline(always)] fn check_relocations(&self, cx: &impl HasDataLayout, range: AllocRange) -> AllocResult { - if self.get_relocations(cx, range).is_empty() { - Ok(()) - } else { - Err(AllocError::ReadPointerAsBytes) - } + if self.has_relocations(cx, range) { Err(AllocError::ReadPointerAsBytes) } else { Ok(()) } } /// Removes all relocations inside the given range. From 18cbc19de2947008651649bd559f44a5db540d18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Mon, 13 Jun 2022 15:19:18 +0200 Subject: [PATCH 3/6] ctfe: clarify skipping the interning walk Reorganizes the previous commits to have a single exit-point to avoid doing the potentially costly walk. Also moves the relocations tests before the interior mutability test: only references are important when checking for `UnsafeCell`s and we're checking if there are any to decide to avoid the walk anyways. --- .../rustc_const_eval/src/interpret/intern.rs | 41 ++++++++++++++----- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs index 808e7db5ae3c0..b243f07edff6a 100644 --- a/compiler/rustc_const_eval/src/interpret/intern.rs +++ b/compiler/rustc_const_eval/src/interpret/intern.rs @@ -173,17 +173,41 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory // // As an optimization, however, if the allocation does not contain any pointers: we don't // need to do the walk. It can be costly for big arrays for example (e.g. issue #93215). + let is_walk_needed = |mplace: &MPlaceTy<'tcx>| -> InterpResult<'tcx, bool> { + // ZSTs cannot contain pointers, we can avoid the interning walk. + if mplace.layout.is_zst() { + return Ok(false); + } + + // Now, check whether this alloc contains reference types (as relocations). + + // FIXME(lqd): checking the size and alignment could be expensive here, only do the + // following for the potentially bigger aggregates like arrays and slices. + let Some((size, align)) = self.ecx.size_and_align_of_mplace(&mplace)? else { + // We do the walk if we can't determine the size of the mplace: we may be dealing + // with extern types here in the future. + return Ok(true); + }; - let Some((size, align)) = self.ecx.size_and_align_of_mplace(&mplace)? else { - // We could be dealing with an extern type here in the future, so we do the regular + // If there are no refs or relocations in this allocation, we can avoid the interning // walk. - return self.walk_aggregate(mplace, fields); + if let Some(alloc) = self.ecx.get_ptr_alloc(mplace.ptr, size, align)? + && !alloc.has_relocations() { + return Ok(false); + } + + // In the general case, we do the walk. + Ok(true) }; - let Some(alloc) = self.ecx.get_ptr_alloc(mplace.ptr, size, align)? else { - // ZSTs cannot contain pointers, so we can skip them. + // If this allocation contains no references to intern, we avoid the potentially costly + // walk. + // + // We can do this before the checks for interior mutability below, because only references + // are relevant in that situation, and we're checking if there are any here. + if !is_walk_needed(mplace)? { return Ok(()); - }; + } if let Some(def) = mplace.layout.ty.ty_adt_def() { if Some(def.did()) == self.ecx.tcx.lang_items().unsafe_cell_type() { @@ -198,11 +222,6 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory } } - if !alloc.has_relocations() { - // There are no refs or relocations in this allocation, we can skip the interning walk. - return Ok(()); - } - self.walk_aggregate(mplace, fields) } From c9772d76198ec04c9e610ce5d95bb9453021aa1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Mon, 13 Jun 2022 17:35:00 +0200 Subject: [PATCH 4/6] const alloc interning: only check for references for arrays/slices Checking the size/alignment of an mplace may be costly, so we only do it on the types where the walk we want to avoid could be expensive: the larger types like arrays and slices, rather than on all aggregates being interned. --- .../rustc_const_eval/src/interpret/intern.rs | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs index b243f07edff6a..2655568090c8e 100644 --- a/compiler/rustc_const_eval/src/interpret/intern.rs +++ b/compiler/rustc_const_eval/src/interpret/intern.rs @@ -179,21 +179,25 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory return Ok(false); } - // Now, check whether this alloc contains reference types (as relocations). - - // FIXME(lqd): checking the size and alignment could be expensive here, only do the - // following for the potentially bigger aggregates like arrays and slices. - let Some((size, align)) = self.ecx.size_and_align_of_mplace(&mplace)? else { - // We do the walk if we can't determine the size of the mplace: we may be dealing - // with extern types here in the future. - return Ok(true); - }; + // Now, check whether this allocation contains reference types (as relocations). + // + // Note, this check may sometimes not be cheap, so we only do it when the walk we'd like + // to avoid could be expensive: on the potentially larger types, arrays and slices, + // rather than on all aggregates unconditionally. + if matches!(mplace.layout.ty.kind(), ty::Array(..) | ty::Slice(..)) { + let Some((size, align)) = self.ecx.size_and_align_of_mplace(&mplace)? else { + // We do the walk if we can't determine the size of the mplace: we may be + // dealing with extern types here in the future. + return Ok(true); + }; - // If there are no refs or relocations in this allocation, we can avoid the interning - // walk. - if let Some(alloc) = self.ecx.get_ptr_alloc(mplace.ptr, size, align)? - && !alloc.has_relocations() { - return Ok(false); + // If there are no refs or relocations in this allocation, we can avoid the + // interning walk. + if let Some(alloc) = self.ecx.get_ptr_alloc(mplace.ptr, size, align)? { + if !alloc.has_relocations() { + return Ok(false); + } + } } // In the general case, we do the walk. From 6d03c8d751128ce064fac4f607e5478db41b04b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Tue, 28 Jun 2022 22:08:28 +0200 Subject: [PATCH 5/6] fix comments --- compiler/rustc_const_eval/src/interpret/intern.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs index 2655568090c8e..2f384247caa3b 100644 --- a/compiler/rustc_const_eval/src/interpret/intern.rs +++ b/compiler/rustc_const_eval/src/interpret/intern.rs @@ -168,10 +168,10 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory mplace: &MPlaceTy<'tcx>, fields: impl Iterator>, ) -> InterpResult<'tcx> { - // We want to walk the aggregate to look for reference types to intern. While doing that we + // We want to walk the aggregate to look for references to intern. While doing that we // also need to take special care of interior mutability. // - // As an optimization, however, if the allocation does not contain any pointers: we don't + // As an optimization, however, if the allocation does not contain any references: we don't // need to do the walk. It can be costly for big arrays for example (e.g. issue #93215). let is_walk_needed = |mplace: &MPlaceTy<'tcx>| -> InterpResult<'tcx, bool> { // ZSTs cannot contain pointers, we can avoid the interning walk. @@ -179,7 +179,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory return Ok(false); } - // Now, check whether this allocation contains reference types (as relocations). + // Now, check whether this allocation could contain references. // // Note, this check may sometimes not be cheap, so we only do it when the walk we'd like // to avoid could be expensive: on the potentially larger types, arrays and slices, @@ -191,8 +191,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory return Ok(true); }; - // If there are no refs or relocations in this allocation, we can avoid the - // interning walk. + // If there are no relocations in this allocation, it does not contain references + // that point to another allocation, and we can avoid the interning walk. if let Some(alloc) = self.ecx.get_ptr_alloc(mplace.ptr, size, align)? { if !alloc.has_relocations() { return Ok(false); From d634f14f26dc6640a9b6cd8b5f896aaf1ecc3cb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Tue, 28 Jun 2022 22:25:58 +0200 Subject: [PATCH 6/6] avoid walk when `get_ptr_alloc` returns no `AllocRef` --- compiler/rustc_const_eval/src/interpret/intern.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs index 2f384247caa3b..9997bb2c0fdf6 100644 --- a/compiler/rustc_const_eval/src/interpret/intern.rs +++ b/compiler/rustc_const_eval/src/interpret/intern.rs @@ -197,6 +197,9 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory if !alloc.has_relocations() { return Ok(false); } + } else { + // We're encountering a ZST here, and can avoid the walk as well. + return Ok(false); } }