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

[Hyoun Kyu Cho via Phabricator via llvm-commits]

netforce added a comment.

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

> First off, let me say that I don't feel qualified to set the direction here, nor I am fully familiar with all of these interfaces. However, since I am already involved in here, I am going to say something anyway. :)
>
> The way I would imagine this working ideally is that there would be two layers. The lower layer would provide more explicit access to the debug info, and it would provide it's users with the ability to manually manage the memory usage. It would be the responsibility of the users to not shoot themselves in the foot if they use it.
>
> The second layer would offer a higher level view of the debug info and it would manage the memory (semi)automatically. Being high level it would/could/should also abstract away the difference between the different debug info formats.
>
> The first layer would roughly correspond to the current DWARFUnit, DWARFContext, etc. classes, with one important difference DWARFContext would not implement DIContext -- it would be a standalone class. The second layer would correspond to the DIContext class, and any of the (new) helper classes it needs to manage the memory and perform the abstractions.
>
> The main advantage I see in that is the breaking of the is-a relationship between DWARFContext and DIContext. Without it the interfaces seems rather shoot-footy because one can happily play around with the lower-level DWARFContext apis (which don't manage memory), and then accidentally call some higher level method which comes from DIContext , which does the management and will then cause the debug info to disappear from under you.
>
> As I said, I don't know how easy would be to reach this state, nor whether it would be an acceptable state for other stakeholders...


Hi Pavel,

I like your two layers idea, since it could give better control to the lower level API users and enable the new feature for the higher level API users without complicating the higher level API.

However, I don't think I have enough understanding of the wholesome picture of the DWARF API to make it in a better shape by refactoring it, which I believe is needed to decouple DWARFContext from DIContext. Furthermore, that seems quite out of the scope of this revision.

So, here's my suggestion that could make this revision align better with the direction you suggested, but leaving the refactoring for a later revision from someone who has better knowledge of the API.

1. I'll expose primitive member functions in DWARFUnit and DWARFContext so that lower level API users can control memory management themselves.
2. I'll introduce a new wrapper class (say CachedDWARFContext?) that implements DIContext with the LRU caching and having an instance of DWARFContext.
3. I'll let llvm-symbolizer uses the new wrapper class where it currently uses DWARFContext as DIContext.

I think this is one step closer to what you suggest without letting an unqualified person (= me) substantially modify the API (= DWARFContext) that's used by several different clients. It'll also make it easier to decouple DWARFContext from DIContext in a later revision.

WDYT? If you think this is reasonable, I'll prepare the changes. We can talk more about the details there and ask the other stakeholders if this is OK too.

Thank you,
HK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78950