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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The cache looks way simpler now, thank you!
As discussed offline, flattening ASTBuilder right into ASTWorker still seems like a good idea to me, but happy with what you come up with there.



================
Comment at: clangd/TUScheduler.cpp:71
+
+  /// Update the function used to compute the value.
+  void update(std::function<llvm::Optional<ParsedAST>()> ComputeF);
----------------
ilya-biryukov wrote:
> 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`?
I think the name is actually fine, it's still mostly a cache.
It does have things in common with a pool, but unrelated consumers can't share a resource, so I think that name is at least as misleading.


================
Comment at: clangd/TUScheduler.cpp:66
+
+/// Provides an LRU cache of ASTs.
+class TUScheduler::ASTCache {
----------------
I'd say a little more about the interaction here. e.g. 
```
/// An LRU cache of idle ASTs.
/// Because we want to limit the overall number of these we retain, the cache
/// owns ASTs (and may evict them) while their workers are idle.
/// Workers borrow them when active, and return them when done.


================
Comment at: clangd/TUScheduler.cpp:84
+  /// Store the value in the pool, possibly removing the last used AST.
+  void put(Key K, std::unique_ptr<ParsedAST> V) {
+    std::unique_lock<std::mutex> Lock(Mut);
----------------
consider assert(findByKey(K) == LRU.end()) as a precondition


================
Comment at: clangd/TUScheduler.cpp:92
+    LRU.pop_back();
+    // AST destructor may need to run, make sure it happens outside the lock.
+    Lock.unlock();
----------------
Just "run the expensive destructor outside the lock"?
the "may not" case seems unimportant and slightly confusing here


================
Comment at: clangd/TUScheduler.cpp:94
+    Lock.unlock();
+    ForCleanup.reset();
+  }
----------------
this line isn't actually needed right?


================
Comment at: clangd/TUScheduler.cpp:342
+    if (!AST)
+      return Action(llvm::make_error<llvm::StringError>(
+          "invalid AST", llvm::errc::invalid_argument));
----------------
This failure doesn't get cached, correct? That's bad for performance.

But if we think this is always a clangd bug, it's probably fine. (and certainly simplifies things)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063