Skip to content

Commit

Permalink
Auto merge of rust-lang#2865 - Vanille-N:tb-perf, r=RalfJung
Browse files Browse the repository at this point in the history
Thorough merge after GC: fix of rust-lang#2863

Context: rust-lang#2863.

`perf report`s of `MIRIFLAGS=-Zmiri-tree-borrows cargo +miri miri test test_invalid_name_lengths` in crate `http`:

### Pre
```
  91.06%  rustc    miri                                 [.] miri::borrow_tracker::tree_borrows::tree::Tree::keep_only_needed
   2.99%  rustc    miri                                 [.] miri::borrow_tracker::tree_borrows::tree::TreeVisitor::traverse_parents_this
   0.91%  rustc    miri                                 [.] miri::borrow_tracker::tree_borrows::tree::Tree::perform_access
   0.62%  rustc    miri                                 [.] miri::range_map::RangeMap<T>::iter_mut
   0.17%  rustc    libc.so.6                            [.] realloc
   0.14%  rustc    miri                                 [.] miri::concurrency::thread::EvalContextExt::run_threads
   0.13%  rustc    miri                                 [.] rustc_const_eval::interpret::operand::<impl rustc_const_eval::interpret::eva
   0.13%  rustc    miri                                 [.] hashbrown::raw::RawTable<T,A>::remove_entry
   0.10%  rustc    miri                                 [.] miri::intptrcast::GlobalStateInner::alloc_base_addr
   0.08%  rustc    librustc_driver-c82c1dc22c817a10.so  [.] <rustc_middle::mir::Body>::source_info
```
Interrupted after 3min 30s.

### Post
```
  20.75%  rustc    miri                                 [.] miri::borrow_tracker::tree_borrows::tree::TreeVisitor::traverse_parents_this
  18.50%  rustc    miri                                 [.] miri::borrow_tracker::tree_borrows::tree::Tree::keep_only_needed
   6.49%  rustc    miri                                 [.] miri::borrow_tracker::tree_borrows::tree::Tree::perform_access
   4.25%  rustc    miri                                 [.] miri::range_map::RangeMap<T>::iter_mut
   1.91%  rustc    libc.so.6                            [.] realloc
   1.79%  rustc    miri                                 [.] miri::concurrency::thread::EvalContextExt::run_threads
   1.40%  rustc    miri                                 [.] rustc_const_eval::interpret::operand::<impl rustc_const_eval::interpret::eva
   1.40%  rustc    miri                                 [.] miri::range_map::RangeMap<T>::merge_adjacent_thorough
   1.34%  rustc    miri                                 [.] miri::intptrcast::GlobalStateInner::alloc_base_addr
   0.90%  rustc    librustc_driver-c82c1dc22c817a10.so  [.] <rustc_middle::ty::context::CtxtInterners>::intern_ty
```
Terminates after 1min 13s.

No significant changes to `./miri bench` in either direction: on small benches not enough garbage accumulates for this to be relevant.
  • Loading branch information
bors committed May 10, 2023
2 parents 84f80f1 + 1a4690d commit 7fb4332
Show file tree
Hide file tree
Showing 6 changed files with 92 additions and 2 deletions.
7 changes: 7 additions & 0 deletions src/tools/miri/bench-cargo-miri/zip-equal/Cargo.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
version = 3

[[package]]
name = "zip-equal"
version = "0.1.0"
8 changes: 8 additions & 0 deletions src/tools/miri/bench-cargo-miri/zip-equal/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[package]
name = "zip-equal"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
22 changes: 22 additions & 0 deletions src/tools/miri/bench-cargo-miri/zip-equal/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
//! This is a pathological pattern in which opportunities to merge
//! adjacent identical items in the RangeMap are not properly detected
//! because `RangeMap::iter_mut` is never called on overlapping ranges
//! and thus never merges previously split ranges. This does not produce any
//! additional cost for access operations, but it makes the job of the Tree Borrows
//! GC procedure much more costly.
//! See https://github.com/rust-lang/miri/issues/2863

const LENGTH: usize = (1 << 14) - 1;
const LONG: &[u8] = &[b'x'; LENGTH];

fn main() {
assert!(eq(LONG, LONG))
}

fn eq(s1: &[u8], s2: &[u8]) -> bool {
if s1.len() != s2.len() {
return false;
}

s1.iter().zip(s2).all(|(c1, c2)| *c1 == *c2)
}
8 changes: 7 additions & 1 deletion src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,13 @@ impl<'tcx> Tree {
/// Integration with the BorTag garbage collector
impl Tree {
pub fn remove_unreachable_tags(&mut self, live_tags: &FxHashSet<BorTag>) {
assert!(self.keep_only_needed(self.root, live_tags)); // root can't be removed
let root_is_needed = self.keep_only_needed(self.root, live_tags); // root can't be removed
assert!(root_is_needed);
// Right after the GC runs is a good moment to check if we can
// merge some adjacent ranges that were made equal by the removal of some
// tags (this does not necessarily mean that they have identical internal representations,
// see the `PartialEq` impl for `UniValMap`)
self.rperms.merge_adjacent_thorough();
}

/// Traverses the entire tree looking for useless tags.
Expand Down
31 changes: 30 additions & 1 deletion src/tools/miri/src/borrow_tracker/tree_borrows/unimap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,42 @@ pub struct UniKeyMap<K> {
}

/// From UniIndex to V
#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Debug, Clone, Eq)]
pub struct UniValMap<V> {
/// The mapping data. Thanks to Vec we get both fast accesses, and
/// a memory-optimal representation if there are few deletions.
data: Vec<Option<V>>,
}

impl<V: PartialEq> UniValMap<V> {
/// Exact equality of two maps.
/// Less accurate but faster than `equivalent`, mostly because
/// of the fast path when the lengths are different.
pub fn identical(&self, other: &Self) -> bool {
self.data == other.data
}

/// Equality up to trailing `None`s of two maps, i.e.
/// do they represent the same mapping ?
pub fn equivalent(&self, other: &Self) -> bool {
let min_len = self.data.len().min(other.data.len());
self.data[min_len..].iter().all(Option::is_none)
&& other.data[min_len..].iter().all(Option::is_none)
&& (self.data[..min_len] == other.data[..min_len])
}
}

impl<V: PartialEq> PartialEq for UniValMap<V> {
/// 2023-05: We found that using `equivalent` rather than `identical`
/// in the equality testing of the `RangeMap` is neutral for most
/// benchmarks, while being quite beneficial for `zip-equal`
/// and to a lesser extent for `unicode`, `slice-get-unchecked` and
/// `backtraces` as well.
fn eq(&self, other: &Self) -> bool {
self.equivalent(other)
}
}

impl<V> Default for UniValMap<V> {
fn default() -> Self {
Self { data: Vec::default() }
Expand Down
18 changes: 18 additions & 0 deletions src/tools/miri/src/range_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,24 @@ impl<T> RangeMap<T> {
};
slice.iter_mut().map(|elem| (elem.range.clone(), &mut elem.data))
}

/// Remove all adjacent duplicates
pub fn merge_adjacent_thorough(&mut self)
where
T: PartialEq,
{
let clean = Vec::with_capacity(self.v.len());
for elem in std::mem::replace(&mut self.v, clean) {
if let Some(prev) = self.v.last_mut() {
if prev.data == elem.data {
assert_eq!(prev.range.end, elem.range.start);
prev.range.end = elem.range.end;
continue;
}
}
self.v.push(elem);
}
}
}

#[cfg(test)]
Expand Down

0 comments on commit 7fb4332

Please sign in to comment.