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

[Sam McCall via Phabricator via cfe-commits]

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)
    Cache.hintUsed(AST.get());
  else
    WeakAST = AST = Cache.put(build...);
  return AST;
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063