[PATCH] D47063: [clangd] Keep only a limited number of idle ASTs in memory

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed May 23 09:49:52 PDT 2018

sammccall added a comment.

Having taken a closer look, I think the cache can be simplified/separated a bit more cleanly by returning shared pointers and not allowing lookups, instead restoring limited ownership in CppFile...

Happy to discuss more, esp if you might disagree :)

Comment at: clangd/ClangdUnit.h:132
-/// Manages resources, required by clangd. Allows to rebuild file with new
-/// contents, and provides AST and Preamble for it.
-class CppFile {
+/// A helper class that handles building preambles and ASTs for a file. Also
+/// adds some logging.
This may be change aversion, but I'm not sure this class does enough after this change - it doesn't store the inputs or the outputs/cache, so it kind of seems like it wants to be a function.

I guess the motivation here is that storing the outputs means dealing with the cache, and the cache is local to TUScheduler.
But `CppFile` is only used in TUScheduler, so we could move this there too? It feels like expanding the scope more than I'd like.

The upside is that I think it's a more natural division of responsibility: `CppFile` could continue to be the "main" holder of the `shared_ptr<Preamble>` (which we don't limit, but share), and instead of `Optional<ParsedAST>` it'd have a `weak_ptr<ParsedAST>` which is controlled and can be refreshed through the cache.

As discussed offline, the cache could look something like:
class Cache {
   shared_ptr<ParsedAST> put(ParsedAST);
   void hintUsed(ParsedAST*); // optional, bumps LRU when client reads
   void hintUnused(ParsedAST*); // optional, releases when client abandons

shared_ptr<ParsedAST> CppFile::getAST() {
  shared_ptr<ParsedAST> AST = WeakAST.lock();
  if (AST)
    WeakAST = AST = Cache.put(build...);
  return AST;

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list