[PATCH] D78950: Adds LRU caching of compile units in DWARFContext.

Tue Apr 28 16:13:16 PDT 2020

netforce added a comment.

In D78950#2007530 <https://reviews.llvm.org/D78950#2007530>, @labath wrote:

> In D78950#2006434 <https://reviews.llvm.org/D78950#2006434>, @aprantl wrote:
>
> > Do you think this is always a win, i.e., also for LLDB, or should this be done in a higher layer?
>
>
> Whether this is a win or not will definitely depend on the usage patterns, and it's very hard to know that for sure before trying it out on a specific use case. My gut feeling would be that this wouldn't help lldb's memory usage much because lldb already stores so much other state, but it may have a big (negative) impact on cpu usage. However, given that this functionality can be turned off (thought that could be better documented) I don't think we need to burden ourselves with that too much with what would happen with lldb in particular.
>
> That said, I'm not really sure what to make of this patch. What was they criterion for choosing where to place the `referCompileUnit` calls ? Right, now they seem to be present on a couple of high-level APIs, but those functions are definitely not the only way to access dwarf units. Indeed, once you start thinking about the lower level APIs, things start to get a lot more fuzzier. And dangerous -- for instance, this makes it very easy to turn a perfectly valid DWARFDie object into a landmine if some operation happens to "garbage-collect" the DWARFDebugInfoEntry object that it refers to.
>
> I think that example would definitely speak for a separate layer with a high level api, which offers only a limited (and more controlled) way of accessing the information in the dwarf file. How feasible is that in the current situation -- I don't know.
>
> In D78950#2006767 <https://reviews.llvm.org/D78950#2006767>, @netforce wrote:
>
> > Although I'm not very familiar with LLDB, if this hurts the performance of it, it can opt out of using the caching by giving negative LRUSize when creating DWARFContext.
>
>
> I think it's important to note here that lldb does not use (most of) llvm dwarf parsing code right now -- it has it's own semi-forked versions of a lot of stuff. However, we are looking into making it reuse more (all?) of llvm dwarf parsing code, which is why it is important to ensure the interface stays flexible enough.

I agree that it's better to keep the interface flexible enough. As I mentioned in previous comment, I'm open to moving this to a different layer. Since I'm new to LLVM, please advise me where would be the best place for this.

Here's my thought process of placing this in DWARFContext. I only looked at two users of this DWARF API (llvm-symbolizer and our own symbolization tool). llvm-symbolizer access it mainly through DIContext interface from which DWARFContext inherits. Our symbolization tool access it via both DWARFContext and DWARFUnit. So, I thought DWARFContext would be a good place to put this in order for both of them to use it. I'm sorry I didn't take a look at LLDB.

With that being said, I'm OK with moving the caching to the symbolizer layer. Since they access the API through DWARFContext, it needs to expose a couple more member functions to make it possible:

1. getCompileUnitForAddress
2. clearLineTableForUnit

Do you think this is better in terms of keeping the API flexible enough? Or do you suggest another way? Please let me know. Thanks.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78950