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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/TUScheduler.cpp:71
+
+  /// Update the function used to compute the value.
+  void update(std::function<llvm::Optional<ParsedAST>()> ComputeF);
----------------
sammccall wrote:
> 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.
SG, let's leave as is.


================
Comment at: clangd/TUScheduler.cpp:94
+    Lock.unlock();
+    ForCleanup.reset();
+  }
----------------
sammccall wrote:
> this line isn't actually needed right?
It isn't. But it makes an important operation (destructor of the AST) explicit, so I'd still keep it.


================
Comment at: clangd/TUScheduler.cpp:342
+    if (!AST)
+      return Action(llvm::make_error<llvm::StringError>(
+          "invalid AST", llvm::errc::invalid_argument));
----------------
sammccall wrote:
> 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)
Thanks, good catch! I somehow missed it, since at some point the failure **was** cached.
I think we should cache it.

Failing ASTs is a clangd bug, of course.
However, they might be hard to fix if it's something inside clang, so I believe we should handle the failures gracefully in that case.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063