Repository navigation
Store evicted cache entries to avoid dropping in-use mappings - #759
ChrisDenton wants to merge 1 commit into
Conversation
|
So the immediate thought I had when the issue came up was that maybe we should just wholesale duplicate the cache with every call to |
|
Hm, duplicating everything would be a bit much I think? Debug info can be quite large. That being so, I think we need a reference count somewhere to avoid dropping memory that's in use. But it could be on the |
|
Or, hm, maybe we can store the library id that is in use and restore them by pushing to the cache normally. Something like this: ChrisDenton@8bcfb50? |
|
I did mean something trying to preserve identity without like redoing mmaps, yes. |
988d6a8 to
b72836b
Compare
This makes recursively calling `resolve` safe even if the cache is filled.
ChrisDenton
changed the title
|
Ok, I've taken another go at it. This time it stores then restores evicted cache entries. (btw the macos runner is still intermittently playing up) |
workingjubilee
left a comment
workingjubilee
left a comment
There was a problem hiding this comment.
Thinking about this, as I mentioned, we would also need to make clear_symbol_cache's public version an unsafe fn with the precondition of "no recursion please". A purely reference-counting-based approach doesn't have this disadvantage, I think, insofar as it is a disadvantage.
| let mut last = MaybeUninit::new(value); | ||
| for elem in self.arr[0..len_to_init].iter_mut().rev() { | ||
| // OPT(size): using `mem::swap` allows surprising size regressions | ||
| last = mem::replace(elem, last); |
There was a problem hiding this comment.
brain: "what if this value isn't initialized?"
also brain: "...it's MaybeUninit?"
brain: "...oh, right."
( feel free to ignore this comment. )
| if N == 0 { | ||
| return; | ||
| } else if self.len == N { | ||
| self.len = N - 1; |
There was a problem hiding this comment.
also also brain: "ooh, what if we replace this with a single condition via if let Some(n) = self.len.checked_sub(1) {..."
also also also brain: "...I know backtrace is practically an IOCCC entry, but do you really think that better fits the library?"
( feel free to ignore this comment. )
| // If the cache needs to evict an entry to add a new one, we store | ||
| // the evicted entry so we can restore it in case of recursion. | ||
| struct CacheGuard<'a>(&'a mut Cache, Option<(usize, Mapping)>); | ||
| impl Drop for CacheGuard<'_> { | ||
| fn drop(&mut self) { | ||
| if let Some(entry) = self.1.take() { | ||
| self.0.mappings.push_back(entry); | ||
| } | ||
| } | ||
| } | ||
| let mut guard = CacheGuard(cache, None); | ||
| let cache = &mut guard.0; | ||
|
|
There was a problem hiding this comment.
Yeah, this was basically what I was thinking of, though I'm wondering if storing only one mapping in CacheGuard is enough? The recursion is messing with my head a little on understanding this. Is there a case where during the call we can evict more than one...?
This is one approach to workaround #758.
A reference count is kept with each mapping so we know if it's still in use. If it is then we never drop it from the cache. This will work for recursion up to
MAPPINGS_CACHE_SIZE(aka 4). After that it will fail to add new mappings. Which means it will effectively act as though loading the debug information failed (i.e. it won't resolve any symbols).The disadvantage of this approach is it does potentially break programs that worked before. If you only use a symbol before you recurse (and never afterward) then you could previously do so infinitely*. I'm not sure that's a useful property though and tbh is a bit dubious soundness wise anyway (but probably works in practice).
Other possible approaches include making some kind of linked list of lru caches or redesigning the API so that
Symbolcan properly hold (and drop!) a reference to its cache entry.