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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks a lot better!
There do seem to be a lot of little classes that exist exactly one-per-TU (ASTWorker, ASTBuilder, CachedAST, to a lesser extent ParsedAST) and I'm not sure the balance of responsibilities is quite right. Some comments below.



================
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.
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > 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;
> > }
> > ```
> I've reimplemented the cache with weak_ptr/shared_ptr. 
> With a slightly different interface to hide more stuff in the cache API. Please take a look and let me know what you think.
> 
> Nevertheless, I still find `CppFile` refactoring useful. It unties preambles from the ASTs and I believe this is a good thing, given that their lifetimes are different.
I'm not sure how much they were tangled before, they were computed in the same place, but accessed separately, and it's not sure you ever *want* to compute them separately? (possibly in unit tests?)

I do think making ASTWorker maintain the old preamble and pass it in is confusing. The remaining members are trivial and unrelated enough that I do think if the references to the preamble/ast are removed, then moving the remaining members to ASTWorker or to a dumb struct, and making these free functions would make it easier to navigate the class structure.


================
Comment at: clangd/TUScheduler.cpp:71
+
+  /// Update the function used to compute the value.
+  void update(std::function<llvm::Optional<ParsedAST>()> ComputeF);
----------------
I think I understand this more as "updates the value" but the value is lazy...

I find this API somewhat hard to follow, maybe just because it's unfamiliar.
I've mostly seen cache APIs look like one of:
1. `Cache(function<Value(Input)> Compute)`, `Value Cache::get(Input)`
2. `void Cache::put(Key, Value)`, `Optional<Value> Cache::get(Key)`
3. `Handle Cache::put(Value)`, `Optional<Value> Handle::get()`

This one is `Slot Cache::create()`, `void Slot::update(function<Value()> LazyV)`, `Value Slot::get()`.

It's similar-ish to 3, but has 3 nontrivial operations instead of 2, and the slot object is externally mutable instead of immutable, so it seems more complex. What does it buy us in exchange?

(1 & 2 work well with a natural key or inputs that are easy to compare, which we don't particularly have)


================
Comment at: clangd/TUScheduler.cpp:91
+/// Provides an LRU cache of ASTs.
+class TUScheduler::IdleASTs {
+  friend class CachedAST;
----------------
naming: `IdleASTs` doesn't mention the function of this class, which is a cache.
I'd suggest swapping the names: call the class `ASTCache` and the *instance* `IdleASTs` as that reflects its role within the TUScheduler.

For `CachedAST`, I think the relationship would be well-exposed by nesting it as `ASTCache::Entry`. This also gives you the `friend` for free, which seems like a hint that it's an appropriate structure.
(Though I'm not sure CachedAST is that useful)


================
Comment at: clangd/TUScheduler.cpp:153
+  /// one.
+  std::vector<std::pair<CachedAST *, std::shared_ptr<llvm::Optional<ParsedAST>>>>
+      LRU; /* GUARDED_BY(Mut) */
----------------
as discussed offline, using a CachedAST* as a key shouldn't be necessary, the ParsedAST* should be enough I think.


================
Comment at: clangd/TUScheduler.cpp:157
+
+CachedAST::~CachedAST() { Owner.remove(*this); }
+
----------------
document why


================
Comment at: clangd/TUScheduler.h:48
+/// kept in memory.
+struct ASTRetentionParams {
+  /// Maximum number of ASTs to be retained in memory when there are no pending
----------------
nit: `Policy` would be more specific than `Params`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063