[Lldb-commits] [PATCH] D113789: Add the ability to cache information for a module between debugger instances.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Maybe it's good for starting a discussion, but I am surprised that we would need to cache symtab information. This is a fairly simple format that needs to be read at application runtime as well, so I am surprised that reading it from the cache is substantially faster than doing it from the object file. What is the slow part there? Is it demangling by any chance?

Now to the discussion. Overall, I think this is an interesting feature, but I do have some remarks/questions:

- I don't think that  "lldb module cache" is very good name for this feature as we already have the "platform module cache" feature (see `Target/ModuleCache.h` and its callers), which deals with downloading (and caching) modules (object files) from remote systems. And then there's the clang module cache, of course.. (and the llvm modules, but we fortunately don't cache those). It would be better if we chose a slightly less overloaded name. Maybe "index cache" ? (I assume you will also want to store dwarf indexes there)
- I don't see anything which would ensure cache consistency for the case where multiple debugger instances try to cache the same module simultaneously. What's the plan for that?
- Have you considered the building upon the caching infrastructure in llvm (`llvm/Caching.h`, `llvm/CachePruning.h`)? I believe it already has some size limiting features and multiprocess synchronization built-in, so using it would take care of the previous item, and of @wallace's size limit request.
- it's moderately strange to be using the gsym file writer for this purpose

I haven't looked at the code in detail, but I noted that the filesystem changes, and some of the unrelated formatting cleanups could be put into separate patches.



================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2722
       m_symtab_up = std::make_unique<Symtab>(symtab->GetObjectFile());
+      if (m_symtab_up->LoadFromCache())
+        return m_symtab_up.get();
----------------
Can we make it so that we don't need to add these bits to every object file implementation? Maybe create a protected `CreateSymtab` function, which would get only called if the Symtab really needs to be created?


================
Comment at: lldb/source/Symbol/Symbol.cpp:645
+  file.writeU16(m_type_data);
+  file.writeU16((&m_type_data)[1]);
+  m_mangled.Encode(file);
----------------
It would be (much) better to put the bitfields in an (anonymous) union that can be accessed both as a uint16_t and as individual bitfields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113789