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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I have addressed the comments regarding the cache implementation. ASTBuilder ones are still pending, but I would appreciate the feedback on how `TUScheduler.cpp` looks like.



================
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.
----------------
sammccall wrote:
> 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.
> it's not sure you ever *want* to compute them separately?
I think that's exactly what we're doing now. The ASTs can now get built separately from the preamble, because they can be evicted from the cache and need to be rebuild while  preamble is not changed.

> 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.
Dumb struct SG, it's essentially what ASTWorker is right now.


================
Comment at: clangd/TUScheduler.cpp:71
+
+  /// Update the function used to compute the value.
+  void update(std::function<llvm::Optional<ParsedAST>()> ComputeF);
----------------
sammccall wrote:
> 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)
As discussed offline, now we have a simpler version that keeps `unique_ptr`s to idle ASTs and the clients are responsible for building the ASTs.
Note that it's not a "cache" per se, so we might want a different name for that.
@sammccall, you suggested to call it a pool, I find it reasonable.  Should we name it `ASTPool` instead of `ASTCache`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063