[PATCH] D119784: [Symbolize] LRU cache binaries in llvm-symbolizer.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D119784#3339028 <https://reviews.llvm.org/D119784#3339028>, @mysterymath wrote:

> In D119784#3334359 <https://reviews.llvm.org/D119784#3334359>, @dblaikie wrote:
>
>> Or would it be OK to evict the last thing even as the next thing is being added to the cache - before all the derived objects are created from the new object? Would that be adequate?
>
> I think it'd be pretty reasonable to load in the second binary before we evict the first; mapping in two large binaries isn't all that worse than one, especially since there currently isn't any bound at all.
> If that's OK, we can get this effect by just always keeping the most recently used binary around. This should be much simpler to code, and it'll only slightly delay evicting the binary to the next `pruneCache()` call.

Hmm, not sure I'm following why that'd be easier to code - is the complexity related to one symbolizer query (the current cache invalidation granularity proposed in this patch, if I understand correctly) maybe needing more than one binary to be loaded, so we can't do any cache invalidation on a finer granularity because of the risk of referenced entities in the invalidated elements in the case of a symbolizer query needing more than one thing kept alive somehow?

Sorry, maybe I'm just a bit brain-fried at the moment and having trouble following something. I /think/ I'm imagining two things, perhaps the one you're proposing already - pruneCache stopping at `LRUBinaries.size() == 1` instead of empty, and possibly doing pruneCache in the recordAccess call instead of having a separate call outside in llvm-symbolizer.cpp? I guess that comes back to the point you made in the review that the invalidation has to be in the client because otherwise stuff will be invalidated out from underneath the client's use case.

Yeah, maybe just changing the empty check to the <= 1 test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119784/new/

https://reviews.llvm.org/D119784