Skip to content

Commit 7fb4332

Browse files
committed
Auto merge of rust-lang#2865 - Vanille-N:tb-perf, r=RalfJung
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.
2 parents 84f80f1 + 1a4690d commit 7fb4332

File tree

6 files changed

+92
-2
lines changed

6 files changed

+92
-2
lines changed
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# This file is automatically @generated by Cargo.
2+
# It is not intended for manual editing.
3+
version = 3
4+
5+
[[package]]
6+
name = "zip-equal"
7+
version = "0.1.0"
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
[package]
2+
name = "zip-equal"
3+
version = "0.1.0"
4+
edition = "2021"
5+
6+
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
7+
8+
[dependencies]
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
//! This is a pathological pattern in which opportunities to merge
2+
//! adjacent identical items in the RangeMap are not properly detected
3+
//! because `RangeMap::iter_mut` is never called on overlapping ranges
4+
//! and thus never merges previously split ranges. This does not produce any
5+
//! additional cost for access operations, but it makes the job of the Tree Borrows
6+
//! GC procedure much more costly.
7+
//! See https://github.com/rust-lang/miri/issues/2863
8+
9+
const LENGTH: usize = (1 << 14) - 1;
10+
const LONG: &[u8] = &[b'x'; LENGTH];
11+
12+
fn main() {
13+
assert!(eq(LONG, LONG))
14+
}
15+
16+
fn eq(s1: &[u8], s2: &[u8]) -> bool {
17+
if s1.len() != s2.len() {
18+
return false;
19+
}
20+
21+
s1.iter().zip(s2).all(|(c1, c2)| *c1 == *c2)
22+
}

src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,13 @@ impl<'tcx> Tree {
488488
/// Integration with the BorTag garbage collector
489489
impl Tree {
490490
pub fn remove_unreachable_tags(&mut self, live_tags: &FxHashSet<BorTag>) {
491-
assert!(self.keep_only_needed(self.root, live_tags)); // root can't be removed
491+
let root_is_needed = self.keep_only_needed(self.root, live_tags); // root can't be removed
492+
assert!(root_is_needed);
493+
// Right after the GC runs is a good moment to check if we can
494+
// merge some adjacent ranges that were made equal by the removal of some
495+
// tags (this does not necessarily mean that they have identical internal representations,
496+
// see the `PartialEq` impl for `UniValMap`)
497+
self.rperms.merge_adjacent_thorough();
492498
}
493499

494500
/// Traverses the entire tree looking for useless tags.

src/tools/miri/src/borrow_tracker/tree_borrows/unimap.rs

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,42 @@ pub struct UniKeyMap<K> {
3636
}
3737

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

46+
impl<V: PartialEq> UniValMap<V> {
47+
/// Exact equality of two maps.
48+
/// Less accurate but faster than `equivalent`, mostly because
49+
/// of the fast path when the lengths are different.
50+
pub fn identical(&self, other: &Self) -> bool {
51+
self.data == other.data
52+
}
53+
54+
/// Equality up to trailing `None`s of two maps, i.e.
55+
/// do they represent the same mapping ?
56+
pub fn equivalent(&self, other: &Self) -> bool {
57+
let min_len = self.data.len().min(other.data.len());
58+
self.data[min_len..].iter().all(Option::is_none)
59+
&& other.data[min_len..].iter().all(Option::is_none)
60+
&& (self.data[..min_len] == other.data[..min_len])
61+
}
62+
}
63+
64+
impl<V: PartialEq> PartialEq for UniValMap<V> {
65+
/// 2023-05: We found that using `equivalent` rather than `identical`
66+
/// in the equality testing of the `RangeMap` is neutral for most
67+
/// benchmarks, while being quite beneficial for `zip-equal`
68+
/// and to a lesser extent for `unicode`, `slice-get-unchecked` and
69+
/// `backtraces` as well.
70+
fn eq(&self, other: &Self) -> bool {
71+
self.equivalent(other)
72+
}
73+
}
74+
4675
impl<V> Default for UniValMap<V> {
4776
fn default() -> Self {
4877
Self { data: Vec::default() }

src/tools/miri/src/range_map.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,24 @@ impl<T> RangeMap<T> {
227227
};
228228
slice.iter_mut().map(|elem| (elem.range.clone(), &mut elem.data))
229229
}
230+
231+
/// Remove all adjacent duplicates
232+
pub fn merge_adjacent_thorough(&mut self)
233+
where
234+
T: PartialEq,
235+
{
236+
let clean = Vec::with_capacity(self.v.len());
237+
for elem in std::mem::replace(&mut self.v, clean) {
238+
if let Some(prev) = self.v.last_mut() {
239+
if prev.data == elem.data {
240+
assert_eq!(prev.range.end, elem.range.start);
241+
prev.range.end = elem.range.end;
242+
continue;
243+
}
244+
}
245+
self.v.push(elem);
246+
}
247+
}
230248
}
231249

232250
#[cfg(test)]

0 commit comments

Comments
 (0)