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

[Hyoun Kyu Cho via Phabricator via llvm-commits]

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?


When we decide where to put caching, I'll see if I can add some unit tests.



================
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;
----------------
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.


================
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);
+
----------------
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.


================
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);
+
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78950