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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu May 24 02:01:19 PDT 2018


ilya-biryukov added inline comments.


================
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:
> 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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063





More information about the cfe-commits mailing list