[PATCH] D42573: [wip] The new threading implementation

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/TUScheduler.h:63
+      Context Ctx, ParseInputs Inputs,
+      UniqueFunction<void(Context, llvm::Optional<std::vector<DiagWithFixIts>>)>
+          OnUpdated);
----------------
sammccall wrote:
> do we want to move the callback to be a clangd-global thing, rather than a per-update thing, before this patch or later?
> 
> (And will that change be mirrored here, or will FileASTThread::update still take a callback?)
I think we're fine both ways.
Those changes are pretty much independent of the other parts of the code. 
I'd personally first finish this patch and remove the callback afterwards. But I could also later update the patch to have the new callback semantics if you beat me to it.


================
Comment at: clangd/TUScheduler.h:70
+  mutable std::mutex Mutex;
+  bool Done;
+  std::condition_variable RequestsCV;
----------------
sammccall wrote:
> (no action required, just random thoughts)
> 
> one day we should really get the threading annotations (REQUIRES_EXCLUSIVE_LOCK etc macros) set up in LLVM - they're *implemented* in llvm for crying out loud...
Yeah, they're certainly great. The sole reason we don't have them enabled is because LLVM does not use threading that much?


================
Comment at: clangd/TUScheduler.h:72
+  std::condition_variable RequestsCV;
+  RequestQueue Requests;
+  std::shared_ptr<CppFile> File;
----------------
sammccall wrote:
> Having trouble seeing which parts of this are really used.
> My understanding is that the ThreadPool+Queue abstraction is going away.
> This means this is a deque + lock + ... a "done" state of some kind? Is more of it still live?
> 
> I'd probably expect the 'done' state to be redundant with the one here... If it's just a deque + a lock, I'd lean towards making it really thin, maybe even a plain struct, and putting it in this file.
> 
> But not sure of your thoughts here.
I'll probably remove `RequestsQueue` or turn it into something really simple like you suggest.
It certainly has more state than we need with this patch. This is one of the reasons this patch is far from being ready.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42573