From 63b7d260a9be99eae43f41a2fc5df5049045e357 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Thu, 2 Jan 2025 17:17:13 -0800 Subject: [PATCH 1/4] Use a custom container for Cache's cache The new Lru type, as a single-purpose "ArrayVec", - removes a level of indirection - omits a needless Vec allocation entirely - internalizes invariants we attempted to maintain in open-coded form It costs a little `unsafe` to handle Drop, which we unfortunately need as addr2line uses Arc inside Context. --- src/symbolize/gimli.rs | 38 ++++++++------------ src/symbolize/gimli/lru.rs | 73 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 24 deletions(-) create mode 100644 src/symbolize/gimli/lru.rs diff --git a/src/symbolize/gimli.rs b/src/symbolize/gimli.rs index b3b8d057..31b83b51 100644 --- a/src/symbolize/gimli.rs +++ b/src/symbolize/gimli.rs @@ -52,8 +52,11 @@ cfg_if::cfg_if! { } } +mod lru; mod stash; +use lru::Lru; + const MAPPINGS_CACHE_SIZE: usize = 4; struct Mapping { @@ -263,7 +266,7 @@ struct Cache { /// /// Note that this is basically an LRU cache and we'll be shifting things /// around in here as we symbolize addresses. - mappings: Vec<(usize, Mapping)>, + mappings: Lru<(usize, Mapping), MAPPINGS_CACHE_SIZE>, } struct Library { @@ -344,7 +347,7 @@ pub unsafe fn clear_symbol_cache() { impl Cache { fn new() -> Cache { Cache { - mappings: Vec::with_capacity(MAPPINGS_CACHE_SIZE), + mappings: Lru::default(), libraries: native_libraries(), } } @@ -403,31 +406,18 @@ impl Cache { } fn mapping_for_lib<'a>(&'a mut self, lib: usize) -> Option<(&'a mut Context<'a>, &'a Stash)> { - let idx = self.mappings.iter().position(|(idx, _)| *idx == lib); - - // Invariant: after this conditional completes without early returning - // from an error, the cache entry for this path is at index 0. + let cache_idx = self.mappings.iter().position(|(lib_id, _)| *lib_id == lib); - if let Some(idx) = idx { - // When the mapping is already in the cache, move it to the front. - if idx != 0 { - let entry = self.mappings.remove(idx); - self.mappings.insert(0, entry); - } + let cache_entry = if let Some(idx) = cache_idx { + self.mappings.move_to_front(idx) } else { - // When the mapping is not in the cache, create a new mapping, - // insert it into the front of the cache, and evict the oldest cache - // entry if necessary. - let mapping = create_mapping(&self.libraries[lib])?; - - if self.mappings.len() == MAPPINGS_CACHE_SIZE { - self.mappings.pop(); - } - - self.mappings.insert(0, (lib, mapping)); - } + // When the mapping is not in the cache, create a new mapping and insert it, + // which will also evict the oldest entry. + create_mapping(&self.libraries[lib]) + .and_then(|mapping| self.mappings.push_front((lib, mapping))) + }; - let mapping = &mut self.mappings[0].1; + let (_, mapping) = cache_entry?; let cx: &'a mut Context<'static> = &mut mapping.cx; let stash: &'a Stash = &mapping.stash; // don't leak the `'static` lifetime, make sure it's scoped to just diff --git a/src/symbolize/gimli/lru.rs b/src/symbolize/gimli/lru.rs new file mode 100644 index 00000000..9f38cd74 --- /dev/null +++ b/src/symbolize/gimli/lru.rs @@ -0,0 +1,73 @@ +use core::mem::{self, MaybeUninit}; +use core::ptr; + +/// least-recently-used cache with static size +pub(crate) struct Lru { + // SAFETY: len <= initialized values + len: usize, + arr: [MaybeUninit; N], +} + +impl Default for Lru { + fn default() -> Self { + Lru { + len: 0, + arr: [const { MaybeUninit::uninit() }; N], + } + } +} + +impl Lru { + #[inline] + pub fn clear(&mut self) { + let len = self.len; + self.len = 0; + // SAFETY: we can't touch these values again due to setting self.len = 0 + unsafe { ptr::drop_in_place(ptr::addr_of_mut!(self.arr[0..len]) as *mut [T]) } + } + + #[inline] + pub fn iter(&self) -> impl Iterator { + self.arr[0..self.len] + .iter() + // SAFETY: we only iterate initialized values due to our len invariant + .map(|init| unsafe { init.assume_init_ref() }) + } + + #[inline] + pub fn push_front(&mut self, value: T) -> Option<&mut T> { + if N == 0 { + return None; + } else if self.len == N { + self.len = N - 1; + // SAFETY: we maintain len invariant and bail on N == 0 + unsafe { ptr::drop_in_place(self.arr.as_mut_ptr().cast::().add(N - 1)) }; + }; + let len_to_init = self.len + 1; + let mut last = MaybeUninit::new(value); + for elem in self.arr[0..len_to_init].iter_mut() { + last = mem::replace(elem, last); + } + self.len = len_to_init; + + self.arr + .first_mut() + // SAFETY: we just pushed it + .map(|elem| unsafe { elem.assume_init_mut() }) + } + + #[inline] + pub fn move_to_front(&mut self, idx: usize) -> Option<&mut T> { + let elem = self.arr[0..self.len].get_mut(idx)?; + // SAFETY: we already bailed if the index is bad, so our slicing will be infallible, + // so it is permissible to allow the len invariant to decay, as we always restore it + let mut last = mem::replace(elem, MaybeUninit::uninit()); + for elem in self.arr[0..=idx].iter_mut() { + last = mem::replace(elem, last); + } + self.arr + .first_mut() + // SAFETY: we have restored the len invariant + .map(|elem| unsafe { elem.assume_init_mut() }) + } +} From 3afccb194e7bd4e658c491ded065908a43709e42 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 3 Jan 2025 16:30:33 -0800 Subject: [PATCH 2/4] raise backtrace MSRV to allow `inline_const` --- .github/workflows/main.yml | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7d0719b4..4330d2f8 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -256,7 +256,7 @@ jobs: with: submodules: true - name: Install Rust - run: rustup update 1.73.0 --no-self-update && rustup default 1.73.0 + run: rustup update 1.79.0 --no-self-update && rustup default 1.79.0 - run: cargo build miri: diff --git a/Cargo.toml b/Cargo.toml index 7261ff77..4f373035 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,7 +14,7 @@ autoexamples = true autotests = true edition = "2021" exclude = ["/ci/"] -rust-version = "1.65.0" +rust-version = "1.79.0" [workspace] members = ['crates/cpp_smoke_test', 'crates/as-if-std'] From b7fbd046949ce63d3f589b475d79a44fffe26ab1 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 3 Jan 2025 17:30:29 -0800 Subject: [PATCH 3/4] mem::swap, not mem::replace, in LRU backshifts --- src/symbolize/gimli/lru.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/symbolize/gimli/lru.rs b/src/symbolize/gimli/lru.rs index 9f38cd74..37ba4a77 100644 --- a/src/symbolize/gimli/lru.rs +++ b/src/symbolize/gimli/lru.rs @@ -46,7 +46,7 @@ impl Lru { let len_to_init = self.len + 1; let mut last = MaybeUninit::new(value); for elem in self.arr[0..len_to_init].iter_mut() { - last = mem::replace(elem, last); + mem::swap(elem, &mut last); } self.len = len_to_init; @@ -63,7 +63,7 @@ impl Lru { // so it is permissible to allow the len invariant to decay, as we always restore it let mut last = mem::replace(elem, MaybeUninit::uninit()); for elem in self.arr[0..=idx].iter_mut() { - last = mem::replace(elem, last); + mem::swap(elem, &mut last); } self.arr .first_mut() From c88b038c345e2ea7465a1b9b01d777f1c93fc6db Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 3 Jan 2025 17:46:16 -0800 Subject: [PATCH 4/4] Revert "mem::swap, not mem::replace, in LRU backshifts" This reverts commit b7fbd046949ce63d3f589b475d79a44fffe26ab1. --- src/symbolize/gimli/lru.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/symbolize/gimli/lru.rs b/src/symbolize/gimli/lru.rs index 37ba4a77..b7cf5a5b 100644 --- a/src/symbolize/gimli/lru.rs +++ b/src/symbolize/gimli/lru.rs @@ -46,7 +46,8 @@ impl Lru { let len_to_init = self.len + 1; let mut last = MaybeUninit::new(value); for elem in self.arr[0..len_to_init].iter_mut() { - mem::swap(elem, &mut last); + // OPT(size): using `mem::swap` allows surprising size regressions + last = mem::replace(elem, last); } self.len = len_to_init; @@ -63,7 +64,8 @@ impl Lru { // so it is permissible to allow the len invariant to decay, as we always restore it let mut last = mem::replace(elem, MaybeUninit::uninit()); for elem in self.arr[0..=idx].iter_mut() { - mem::swap(elem, &mut last); + // OPT(size): using `mem::swap` allows surprising size regressions + last = mem::replace(elem, last); } self.arr .first_mut()