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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
