[PATCH] D42174: [clangd] Refactored threading in ClangdServer

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:222
+  }
+  // We currently do all the reads of the AST on a running thread to avoid
+  // inconsistent states coming from subsequent updates.
----------------
sammccall wrote:
> Having trouble with this one.
> is this the same as "We currently block the calling thread until the AST is available, to avoid..."?
Yes. Used your wording, thanks.


================
Comment at: clangd/ClangdServer.h:169
+  Scheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
+            std::shared_ptr<PCHContainerOperations> PCHs,
+            ASTParsedCallback ASTCallback);
----------------
sammccall wrote:
> this is a performance optimization, right?
> I think the scheduler does enough of the compiling that giving it one of its own isn't too wasteful.
It is probably redundant, yes. Removed it.


================
Comment at: clangd/ClangdServer.h:173
+  /// Get the CompileCommand passed at the last scheduleUpdate call for \p File.
+  llvm::Optional<tooling::CompileCommand> getLatestCommand(PathRef File) const;
+
----------------
sammccall wrote:
> This seems like a smell - the command is sometimes an input from callsites that want builds, and sometimes an output to callsites that want builds. Also this class is all about threading, scheduling, and versions, and this method bypasses all of that.
> 
> (If this is a hack to keep code complete working while we refactor, that's fine - but the docs should say that and we should know what next steps look like!)
> 
> ISTM the issue here is coupling updates to the source with updates to the compile command.
> Ultimately we should indeed be compiling a given version with a fixed CC, but that doesn't seem like the clearest interface for callers.
> 
> One option is:
>   - Scheduler has a reference to the CDB (or a std::function wrapping it), and needs to come up with commands itself
>   - it caches commands whenever it can, and has an "invalidateCommand(Path)" to drop its cache. `with_file_lock { invalidateCommand(), scheduleUpdate() }` is the equivalent to `forceRebuild`.
>  - it provides a `scheduleSourceRead` which is like scheduleASTRead but provides source, compile command, and the latest preamble available without blocking. This would be used for operations like codecomplete that can't use scheduleASTRead.
I've opted for passing action inputs to the read methods, as discussed offline. The implementation turned out to be ugly, we now store a separate `StringMap<ParseInputs>` in `Scheduler` to keep track of the latest inputs and not put that into `CppFile`.


================
Comment at: clangd/ClangdServer.h:184
+  /// resources. \p Action will be called when resources are freed.
+  /// If an error occurs during processing, it is forwarded to the \p Action
+  /// callback.
----------------
sammccall wrote:
> The callback here is a bit confusing and YAGNI (both clangd and our known embedders discard the returned future).
> It seems enough to return synchronously and know that subsequent reads that ask for = or >= version are going to get nothing back.
I'm fine with removing the callback, but I'd like this change to focus on internals of `ClangdServer` without changing its interface, happy to remove the callback afterwards.
Added a FIXME for that.


================
Comment at: clangd/ClangdServer.h:200
+  /// callback.
+  void schedulePreambleRead(
+      PathRef File,
----------------
sammccall wrote:
> can we pass in a consistency policy here? even if we only support a subset of {ast, preamble} x policy for now, a richer API would enable experiments/tradeoffs later. e.g.
> 
>   enum class VersionConstraint {
>     ANY,     // returns the first data that's available, typically immediately
>     EQUAL, // returns the data for the previous update
>     GREATER_EQUAL, // Returns the data for the previous update, or some subsequent update.
>   }
> 
> (I guess the semantics you've described for AST read are EQUAL, and ANY for preamble read)
I would rather not extend the API beyond what it can do at the moment, but happy to explore this approach after threading gets rid of the legacy stuff that we have now.


================
Comment at: clangd/ClangdServer.h:206
+  CppFileCollection Units;
+  SimpleThreadPool Executor;
+};
----------------
sammccall wrote:
> these two names seem a bit confusing - might be easier as just CppFiles, Threads?
> 
> The names seem to be ~synonyms for the types, which I don't think is better than echoing the types. (Often there's a completely separate good name, but I don't think so here)
- I'd go with `Files` instead of `CppFiles`. WDYT?
- `Executor` sounds better to me than `Threads`. Could you elaborate why you find it confusing?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42174