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

[Hyoun Kyu Cho via Phabricator via llvm-commits]

netforce marked 9 inline comments as done.
netforce added a comment.

In D78950#2007001 <https://reviews.llvm.org/D78950#2007001>, @ikudrin wrote:

> Is it possible to add some tests for the LRU logic?


Added a unit test for the LRU logic. Thank you.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:119-126
+  // LRU mechanism of compile units to manage memory usage.
+  using unit_queue_type = std::list<DWARFUnit *>;
+  using unit_queue_iterator = unit_queue_type::iterator;
+  using unit_map_type = std::unordered_map<DWARFUnit *, unit_queue_iterator>;
+  unit_queue_type CompileUnitQueue;
+  unit_map_type CompileUnitMap;
+  const int CompileUnitLRUSize;
----------------
netforce wrote:
> ikudrin wrote:
> > Can this be a separate utility class? No need to overburden `DWARFContext`.
> Once we determine which layer we place this LRU caching first, I'll revisit this and see if it would be cleaner to use a separate utility class.
Added DWARFCachedDIContext to implement DIContext with LRU caching without modifying DWARFContext much. Also added a utility class (DWARFUnitLRUCache) there.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:333
+  // management purpose. When it's referred to again, it'll be re-populated.
+  void clearLineTableForUnit(DWARFUnit *U);
+
----------------
netforce wrote:
> ikudrin wrote:
> > Does this method need to be in the public interface of the class? Right now, it looks like an implementation detail that should be hidden from users.
> I'll revisit this too when we decide which layer to put the caching. For the current implementation, it seems it doesn't need to be public.
To expose memory management feature to higher level API or tools, this needs to be public now. Please refer to the previous discussion for more context.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:416
+  // Refer a compile unit so that it's pushed back in LRU.
+  void referCompileUnit(DWARFUnit *CU);
+
----------------
netforce wrote:
> ikudrin wrote:
> > It looks like this should also be private, no?
> Again, I'll revisit this when we decide which layer to put the caching. For the current implementation, we need this to be public as we have our symbolization tool directly accessing the compile units. 
To implement caching in a separate class, this needs to be public. Actually, it seems it has already become public in another (previous) revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78950