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.

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

Use a custom container for Cache's cache #689

Merged
merged 4 commits into from
Jan 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Expand Down
38 changes: 14 additions & 24 deletions src/symbolize/gimli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,11 @@ cfg_if::cfg_if! {
}
}

mod lru;
mod stash;

use lru::Lru;

const MAPPINGS_CACHE_SIZE: usize = 4;

struct Mapping {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(),
}
}
Expand Down Expand Up @@ -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
Expand Down
75 changes: 75 additions & 0 deletions src/symbolize/gimli/lru.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
use core::mem::{self, MaybeUninit};
use core::ptr;

/// least-recently-used cache with static size
pub(crate) struct Lru<T, const N: usize> {
// SAFETY: len <= initialized values
len: usize,
arr: [MaybeUninit<T>; N],
}

impl<T, const N: usize> Default for Lru<T, N> {
fn default() -> Self {
Lru {
len: 0,
arr: [const { MaybeUninit::uninit() }; N],
Copy link
Member Author

Choose a reason for hiding this comment

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

this requires an MSRV bump, or to use the infamously sus-looking-but-actually-okay-for-this-ONE-instance:

Suggested change
arr: [const { MaybeUninit::uninit() }; N],
arr: unsafe { MaybeUninit::uninit().assume_init() },

Copy link
Member

Choose a reason for hiding this comment

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

On the one hand I'm not against bumping the MSRV for this crate but on the other hand it is only one line. But on the third hand it is a very sussy line that would at least need a comment saying "no really, this is ok!"

What would the new MSRV be?

Copy link
Member Author

Choose a reason for hiding this comment

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

if 318dd91 passes then the answer is 1.79

Copy link
Member

Choose a reason for hiding this comment

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

Ah 1.73 to 1.79? That seems fine to me. We should probably also update the rust-version in Cargo.toml as that seems to have gone out of the sink.

}
}
}

impl<T, const N: usize> Lru<T, N> {
#[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]) }
Copy link
Member

Choose a reason for hiding this comment

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

cast still requires sized, shame.

}

#[inline]
pub fn iter(&self) -> impl Iterator<Item = &T> {
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::<T>().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() {
// OPT(size): using `mem::swap` allows surprising size regressions
last = mem::replace(elem, last);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just mem::swap?

Copy link
Member Author

@workingjubilee workingjubilee Jan 4, 2025

Choose a reason for hiding this comment

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

what we do is simultaneously assign the last to an index, and take out the old value at that index. eventually everything gets moved over one index by this process. it isn't really mem::swap because we're instead moving everything a step over, and that isn't really expressible that way (and there's no windows_mut, besides).

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm having a brain fart but I mean literally last = mem::replace(elem, last); looks suspiciously like mem::swap(elem, &mut last); no?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, that had a big effect on code size???

}
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() {
// OPT(size): using `mem::swap` allows surprising size regressions
last = mem::replace(elem, last);
}
self.arr
.first_mut()
// SAFETY: we have restored the len invariant
.map(|elem| unsafe { elem.assume_init_mut() })
}
}
Loading